Skip to content

Precipitation calculation with isotherms #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

agarciadiego
Copy link
Contributor

@agarciadiego agarciadiego commented Oct 24, 2024

Addresses Issue:

Lack of prediction from previous precipitation data

Summary/Motivation:

Precipitation requires function to calculate precipitation based on acid dosage

Changes proposed in this PR:

-Adds isotherm form calculation for oxalate precipitation based on acid dosage
-Data based on Visual Minteq

Reviewer's checklist / merge requirements:

  • The head branch (i.e. the "source" of the changes) is not the main branch on the PR author's fork
  • Documentation
  • Tests
  • Diagnostic tests for models

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.md file
    at the top level of this directory.

  2. I represent I am authorized to make the contributions and grant the license. If my employer has
    rights to intellectual property that includes these contributions, I represent that I have
    received permission to make contributions and grant the required license on behalf of that
    employer.

@agarciadiego agarciadiego self-assigned this Oct 24, 2024
@agarciadiego agarciadiego added Priority:High High Priority Issue or PR enhancement New feature or request labels Oct 24, 2024
@agarciadiego agarciadiego mentioned this pull request Oct 24, 2024
4 tasks
Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.99%. Comparing base (911fbda) to head (32f2376).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #99   +/-   ##
=======================================
  Coverage   96.98%   96.99%           
=======================================
  Files          48       48           
  Lines        7540     7557   +17     
=======================================
+ Hits         7313     7330   +17     
  Misses        227      227           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor suggestions - I will not formally request changes just in case this is not ready by the time I leave so as not to hold things up with messing around to override my request.

)

self.acid_flow = Var(
units=units.dimensionless,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it really should have dimensions.

Also, how reliably can be define the upper bound for this (which probably depends on the units)?


# parameter based on pH 1.5
# TODO add surrogate model/equation
self.E_D = Param(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more descriptive name and/or a doc string would be good here as I do not really know what I am looking at at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconding this point


# parameter based on pH 1.5
# TODO add surrogate model/equation
self.N_D = Param(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@MarcusHolly
Copy link
Contributor

Seems like this PR still needs to address Andrew's comments as well as resolving some of the flow_mol_comp[Ca(C2O4)] variables being outside of their bounds.

@ksbeattie
Copy link
Contributor

@agarciadiego, any news on this?

Copy link
Contributor

@MarcusHolly MarcusHolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor comments, but I'll start looking into what is needed to incorporate this into the UKy flowsheet today and tomorrow


# parameter based on pH 1.5
# TODO add surrogate model/equation
self.E_D = Param(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconding this point


# parameter based on pH 1.5
# TODO add surrogate model/equation
self.N_D = Param(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Comment on lines +33 to +34
Reaction package for heterogeneous reactions involved in oxalate precipiration of
REEs from solid West Kentucky No. 13 coal refuse using oxalic acid.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Reaction package for heterogeneous reactions involved in oxalate precipiration of
REEs from solid West Kentucky No. 13 coal refuse using oxalic acid.
Reaction package for heterogeneous reactions involved in oxalate precipitation of
REEs from solid West Kentucky No. 13 coal refuse using oxalic acid.

}

# parameter based on pH 1.5
self.E_D = Param(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better name or docstring

)

# parameter based on pH 1.5
self.N_D = Param(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better name or docstring


def build(self):
"""
Reaction block for leaching of West Kentucky No. 13 coal refuse in H2SO4.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this docstring accurate?



# -----------------------------------------------------------------------------
# Leach solution property package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precipitation liquid property package?

# Precipitator unit model
class OxalatePrecipitatorInitializer(ModularInitializerBase):
"""
This is a general purpose Initializer for the Oxalate Precipitator unit model.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This is a general purpose Initializer for the Oxalate Precipitator unit model.
This is a general purpose Initializer for the Oxalate Precipitator unit model.

Comment on lines 1431 to 1435
initializer_precip = OxalatePrecipitatorInitializer()
precip_units = [
m.fs.precipitator,
]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running into the following issue with OxalatePrecipitatorInitializer: idaes.core.util.exceptions.InitializationError: Degrees of freedom for fs.precipitator.mscontactor were not equal to zero during initialization (DoF = 13). The SXInitializer had a similar issue when it was first being developed - see Andrew's comment here on a potential fix: #85 (comment)

@dangunter
Copy link
Contributor

per @MarcusHolly I (@dangunter ) will check how to add new outputs from L1572-2121 in display_results

@dangunter
Copy link
Contributor

dangunter commented Mar 7, 2025

@MarcusHolly I added a version of the uky_flowsheet.py module called uky_flowsheet_new.py that has a refactored display_output method to use an (added) class FlowsheetResuls that calculates and stores the output. You should double-check that this calculation matches the original (I refactored it internally to for less repetition), but if/when it's correct, this will be a good starting point for adding output variables in the UI wrapper module without duplicating any of the code that calculates them.

Once the new version is verified, of course you can replace uky_flowsheet.py with uky_flowsheet_new.py ; I just thought it would be convenient to have both around for comparison for a bit longer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Priority:High High Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants