Skip to content
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

MCAS w/generic crystallizer #1487

Merged
merged 54 commits into from
Oct 31, 2024

Conversation

adam-a-a
Copy link
Contributor

@adam-a-a adam-a-a commented Sep 10, 2024

Fixes/Resolves:

Summary/Motivation:

Building on @OOAmusat 's work on a generic crystallizer by making it compatible with MCAS and also revising to match more closely with IDAES/WaterTAP standards.

Changes proposed in this PR:

  • add enthalpy and pressure_sat relationships for seawater into MCAS
  • remove unnecessary inclusions related to properties or ion sets from crystallizer model and point to prop pack
  • general code cleanup
  • update mcas documentation

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.txt 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.

@adam-a-a adam-a-a changed the title Mcas generic crystallizer Mcas w/generic crystallizer Sep 10, 2024
__author__ = "Oluwamayowa Amusat, Adam Atia"


def add_crystallizer_rbf_model(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking about bringing this back as a method of the 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.

The question is whether we want to do that, or allow the user to define their own function entirely.

I don't think it should be in the unit model file. However, we could support rbfs and maybe even NN models as defaults. We could have the code in a different file that the user can load as the default with optional arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was what I was thinking--something along the lines of calling the method on the unit model to build some surrogate(s). And defaults could sit in our data folder with a new directory for default_surrogates or something like that. Either way, I don't love the idea of it living in this test file or in some future example flowsheet file.

watertap/unit_models/surrogate_crystallizer.py Outdated Show resolved Hide resolved
watertap/unit_models/tests/test_crystallizer_surrogate.py Outdated Show resolved Hide resolved
__author__ = "Oluwamayowa Amusat, Adam Atia"


def add_crystallizer_rbf_model(
Copy link
Contributor

Choose a reason for hiding this comment

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

The question is whether we want to do that, or allow the user to define their own function entirely.

I don't think it should be in the unit model file. However, we could support rbfs and maybe even NN models as defaults. We could have the code in a different file that the user can load as the default with optional arguments.

@kurbansitterley
Copy link
Contributor

@adam-a-a are you only planning on making the surrogate compatible with MCAS or also the physical model?

@adam-a-a
Copy link
Contributor Author

@ElmiraShamlou I can add pressure_sat to MCAS. In that case though, it sounds like we should relate pressure_sat to pressure_operating in the surrogate_crystallizer.

@adam-a-a
Copy link
Contributor Author

I suppose in this case, pressure_operating relates to the surrogate output, so perhaps not. I'd welcome feedback though. Maybe it is enough to just add pressure_sat to MCAS.

@ElmiraShamlou
Copy link
Contributor

@ElmiraShamlou I can add pressure_sat to MCAS. In that case though, it sounds like we should relate pressure_sat to pressure_operating in the surrogate_crystallizer.

@adam-a-a Yes, this is necessary when the surrogate vapor pressure output is unavailable for a generic surrogate. It can be added as needed, using a custom constraint (like blk.pressure_conversion_constraints in the test file), which ties the operating pressure to the saturation pressure. Alternatively, this could be implemented as a config option, with the property package’s saturation vapor pressure set as the default when the surrogate option is not selected.

@ElmiraShamlou
Copy link
Contributor

I suppose in this case, pressure_operating relates to the surrogate output, so perhaps not. I'd welcome feedback though. Maybe it is enough to just add pressure_sat to MCAS.

Yes, it is not required for this specific PR.

@adam-a-a
Copy link
Contributor Author

@ElmiraShamlou That was a good point to bring up though. @OOAmusat is going to put up a PR that adds the reaktoro capability in place of surrogates. I was about to revise the surrogate_crystallizer config options but remembered that we decided to wait on making more changes until after Mayo's upcoming PR. From there, I think we can arrive at a revision that would accommodate the different forms this model can take (e.g., using prop model pressure sat, using surrogate, using reaktoro block, etc.).

@adam-a-a adam-a-a self-assigned this Oct 29, 2024
@adam-a-a adam-a-a changed the title Mcas w/generic crystallizer MCAS w/generic crystallizer Oct 29, 2024
@adam-a-a adam-a-a enabled auto-merge (squash) October 29, 2024 13:40
Copy link
Contributor

@ElmiraShamlou ElmiraShamlou left a comment

Choose a reason for hiding this comment

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

Great job!

@adam-a-a adam-a-a merged commit cbc8882 into watertap-org:main Oct 31, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants