Simple OD Nanofiltration model #1536

andrewlee94 · 2024-12-02T20:33:52Z

Fixes/Resolves: None

Summary/Motivation:

The existing Nanofiltration_ZO model does not preserve electoneutrality in the outlet streams.

Additionally, the MCAS property package does not have a new type Initializer or Scaler.

Changes proposed in this PR:

New Nanofiltration0D unit model with user provided recovery fractions and option to maintain electoneutrality.
Update MCAS properties to use BlockTriangularizationInitializer by default
Adds new MCASScaler for use the new scaling API.

Legal Acknowledgement

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

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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 · 2024-12-02T21:14:50Z

watertap/unit_models/nanofiltration0D.py

+@declare_process_block_class("Nanofiltration0D")
+class Nanofiltration0DData(UnitModelBlockData):


My high-level question before diving in more deeply is-- why do we need another NF model class? Could we instead just modify the existing NF_ZO model and potentially rename if needed? Or could we delete NF_ZO and replace with this?

My concern is that all of the different NF files that we have now can already be confusing, and adding another separate NF file will add to that confusion. So it'd be best if we could consolidate.

After the fact, I am asking myself if it would have been better to merge them. In part I did this to ensure backward compatibility as this model makes a few additional assumptions (both to simplify and to enforce electroneutrality).

adam-a-a

The main thing I think we need to do here is consolidate the two NF models (ZO and this new 0D). After thinking it over, I think the way we should do this is incorporate new changes to the former ZO model (and rename ZO to be 0D) but omit some new config options that deviate from "the standard".

I also had some questions and comments concerning electroneutrality.

docs/technical_reference/unit_models/nanofiltration_0D.rst

adam-a-a · 2024-12-11T15:12:07Z

watertap/property_models/tests/test_multicomp_aq_sol_prop_pack.py

+            assert stream[0].is_property_constructed(v.name)
+
+        initializer = stream.default_initializer()
+        initializer.initialize(stream)


What are your thoughts on asserting a particular record of the initializer? For example, we could use some of the Diagnostic Toolbox functionality to record numerical issues after having applied the initializer. The main idea being that we can leave some form of a benchmark behind in the test, and if we attempt to refine the initializer (or compare with another initializer) in the future, we'd have some sort of metric to compare against and try to improve on.

The first thing I'd want to know is how does the outcome of the initializer compare with the old initialize method?

I am not quite sure what you would be asserting in this case. What is the outcome of an initialization in the general sense? One might say that it is converging the model to a given tolerance, but that is not quite true as it only needs to get it to the point that a full solver can take over (although many of our routines include that as the final step).

TBH- this thought was geared more towards the Scaler originally, but then I figured that we should think about doing some form of a check on Initializers although it is not apparent what the best way to go about it would be, or if it would be worthwhile for something like a property block.

One idea- even though it may not be the best approach- is to check the approximate number of iterations before convergence, which could be a pain between the different runners. Something like that, or we can set a low number on max_iter and check the state of the model. But again--now that I think about it- this probably makes sense for a flowsheet or a complex model and not so much for a more basic model.

Overall though, the main question that I wanted to answer was -- how can we compare two initializers/initialization methods at this level to decide on whether one is superior (for the tested use case)?

That is not easy to define. For one, determining the number of iterations is difficult to start with (as the solvers do not report it), and sometime more iterations are acceptable if it results in greater robustness across a range of conditions. That perhaps is the bigger point - it is hard to mark one initializer as superior to another without considering what they were designed to do (and this is ultimately why IDAES moved to class-based initializers to that we could have multiple options if required).

One thing I can say however is that the number of iterations should be fairly consistent across runners if you have a well scaled problem (but bets are off is the scaling is not good).

adam-a-a · 2024-12-11T15:16:53Z

watertap/property_models/tests/test_multicomp_aq_sol_prop_pack.py

+        set_scaling_factor(stream[0].flow_mass_phase_comp["Liq", "Cl_-"], 1e2)
+        set_scaling_factor(stream[0].flow_mass_phase_comp["Liq", "Mg_2+"], 1e3)
+
+        scaler.scale_model(stream[0])


Similar idea here but probably easier to do-- I am unsure what metric(s) would be best to "benchmark" against, but perhaps asserting the condition number, for example, would be one way of comparing against the performance of the former scaling approach as well as any new scalers (or changes to this one) that might be introduced later.

I am mainly looking for a way to avoid overlooking any potential degradation in model performance/stability as we continue to introduce these scaler classes. Maybe the ScalingProfiler can be used to some extent for this, but I don't think that could apply to comparisons against the old method-based approach for scaling.

For that you want to look at this: https://github.com/IDAES/idaes-compatibility/tree/main/src/idaes_compatibility/robustness

Rather than look at some numerical characteristic of the model, you want to look at how well the solver performed at solving it. Additionally, it cannot be a single point test, but needs to cover a range of conditions.

That would be perfect! I didn't know about this IPOPTConvergenceAnalysis class. Didn't explore its full functionality yet, but if I am understanding correctly, then I would say we should test a range of conditions to check how the scaler affects convergence.

I would even say that we can think about a way to apply that to probing how "good" an initializer is as well, granted we'd have to follow up the initialization with a solve if it wasn't baked in already.

I will note the plan is/was to move IPOPTConvergenceAnalysis to use the parameter sweep tool once it is ready (I tried to make it mostly compatible).

@andrewlee94 which parameter sweep tool? The original one or the one that came out of WaterTAP?

adam-a-a · 2024-12-11T15:20:59Z

watertap/unit_models/nanofiltration_0D.py

+@declare_process_block_class("Nanofiltration0D")
+class Nanofiltration0DData(UnitModelBlockData):
+    """
+    Zero order nanofiltration model.


Suggested change

Zero order nanofiltration model.

Zero dimensional nanofiltration model.

This brings me to my main comment on this PR: we should only have one NF file or the other between the ZO and 0D forms. So, I think it would make sense to get rid of the ZO formulation and incorporate anything from it that is not included in the 0d formulation here.

Actually, it may be better to just incorporate additions made in NF_0D here to the existing NF_ZO formulation (and rename ZO to 0D). I say this after observing deviations in config options and the relation of those deviations to the omission of a ControlVolume.

adam-a-a · 2024-12-11T15:25:04Z

watertap/unit_models/nanofiltration_0D.py

+    CONFIG.declare(
+        "include_pressure_balance",
+        ConfigValue(
+            default=True,
+            domain=Bool,
+            description="Whether to include pressure balance for retentate side.",
+        ),
+    )
+    CONFIG.declare(
+        "has_retentate_pressure_drop",
+        ConfigValue(
+            default=False,
+            domain=Bool,
+            description="Whether to include pressure drop in the retentate side.",
+        ),
+    )
+    CONFIG.declare(
+        "include_temperature_equality",
+        ConfigValue(
+            default=True,
+            domain=Bool,
+            description="Whether to include temperature equality constraints",
+            doc="""Argument indicating whether temperature equality should be
+            inlcluded. If this is False, no energy balance constraint will be
+            written.""",
+        ),


these config options deviate from our typical "standard" and I suppose that is partly because you aren't using a ControlVolume0D in the model. Regardless, I would think that you could at least maintain options like "has_pressure_change". With regard to isothermal constraints, we have an isothermal ControlVolume with config options that allow for doing what "include_temperature_equality" enables.

I did debate this whilst writing the model, and decided to not be overly constrained by standards and instead focus on what made sense for the case at hand. Config option names are easily changed at least, but my initial feeling is that a control volume is unnecessary complications in this case.

If we didn't already have the NF_ZO model, then I would agree. Since we do though, I think the merging of the two should resolve this, and we can just keep the "standard" config options from the ZO formulation while adding your electroneutrality option.

adam-a-a · 2024-12-11T15:33:35Z

watertap/unit_models/nanofiltration_0D.py

+    CONFIG.declare(
+        "electroneutrality_ion",
+        ConfigValue(
+            default="Cl_-",
+            domain=str,
+            description="Balancing ion to use to maintain electroneutrality",
+            doc="""Name of ion to use to maintain electroneutrality in
+            permeate stream. If an ion is provided, the split fraction
+            for this ion will be solved implicitly to ensure electroneutrality
+            is maintained in the permeate stream. If None, user must provide
+            spl;it fractions for all monovalent ions and electroneutrality is
+            not guaranteed..""",
+        ),
+    )


I am thinking that you do not need this option because MCAS will handle electroneutrality for you with the "assert_electroneutrality" method, where you can choose your target ion for rebalancing if needed.

I think I see why you need this now. If one wants to fix split fractions for each ion, one would need to ensure electroneutrality is maintained in the permeate, and choosing the adjustable ion would allow for that to occur. Is that the main intention?

Another thought on this--suppose I have some performance data for my membrane, and I know what the rejection rate (or alternatively solute recovery) is for each ion that I am interested in and have characterized in my solution composition. There could be the case that electroneutrality is not satisfied due to some unmeasured constituent's contribution.

Is your solution to this case to essentially not define any electroneutrality ion, thereby disregarding electroneutrality all together in the permeate? In that case, I wonder if we should log a warning for the user, where we'd check if electroneutrality would be satisfied in the permeate, and provide the warning in the case the electroneutrality would be violated. This way, a user who is already aware of the violation and expects it can wave off the warning, while the naive user who is actually making an unintentional error and would eventually come to the realization that they should balance electroneutrality would get some early notice.

Perhaps this could be injected into the initialization routine (or ignored because we don't know when the user might decide to fix/unfix split fractions.)

Actually, my response in that case is that you need to do data reconciliation to get an electroneutral solution, otherwise your data is not correct.

The model does support None for the electroneutrality species to allow you to forgo that constraint (and replaces it with a standard split fraction) for cases where the user wants to do that. As for doing a check on electroneutrality, to me that is a post-check the user should be doing, as there are too many edge cases that could occur.

docs/technical_reference/unit_models/nanofiltration_0D.rst

adam-a-a · 2024-12-11T15:46:54Z

docs/technical_reference/unit_models/nanofiltration_0D.rst

+   "Solute recovery", ":math:`R_j`", "solute_recovery", [j], ":math:`\text{dimensionless}`"
+   "Multivalent ion recovery", ":math:`R_{multi}`", "multivalent_recovery", None, ":math:`\text{dimensionless}`"


I think solute rejection is more commonly used in practice. The ZO formulation includes rejection, and I think you could keep both when merging the two models.

I decided to only include one of rejection or recovery to avoid any chance of over-specifying the problem - it would be easy to switch to rejection if that would be a better choice. I would generally discourage having both, as someone will attempt to fix both of them and wonder why the model fails to solve.

In that case, we should move to rejection.

Rejection as in 1- permeate_concentration/feed_concentration. This should already be in the ZO model.

watertap/unit_models/tests/test_nanofiltration_0D.py

docs/technical_reference/unit_models/nanofiltration_0D.rst

MarcusHolly · 2025-01-29T16:58:42Z

watertap/property_models/multicomp_aq_sol_prop_pack.py

+        "flow_mass_phase_comp": 1e3,
+        "flow_mol_phase_comp": 1e2,


For Vars such as mass and molar flows that can vary widely in magnitude depending on the specific application, would you recommend setting default scaling factors and have the users manually override the scaling factors themselves, if necessary (as done here), or is would it be better to not set a default scaling factor at all for such variables. One of my concerns is that a user may not know (or simply forget) that they would need to set overwrite=True when trying to adjust these scaling factors themselves (in a flowsheet, for instance). For property packages, I've usually just been applying default scaling factors to flow_vol, temperature, and pressure

Users should always set manual scaling factors before calling automated routines, as those routines will benefit from having better scaling guesses. I.e. they should not be overwriting defaults as they should have set the scaling factors before the defaults exist. Default values should always be seen as a last-ditch effort to provide scaling when the user cannot or does not provide information.

What is needed is good documentation of the tools and expected workflows that users can follow. If they fail to understand how the tools work and use different workflows, then any issues are on the user (we cannot anticipate and protect against every possible course of action).

MarcusHolly · 2025-01-29T17:12:33Z

watertap/property_models/multicomp_aq_sol_prop_pack.py

+        for c in model.component_data_objects(Constraint, descend_into=True):
+            self.scale_constraint_by_nominal_value(
+                c,
+                scheme=ConstraintScalingScheme.inverseMaximum,
+                overwrite=overwrite,
+            )


Are there cases where you wouldn't want to scale all of the constraints like this? When trying this for the anaerobic digester unit model (prob the most finicky model in BSM2), the flowsheet would fail to solve when doing this, so I ended up just scaling none of the constraints by default. I believe I also tried scaling a small subset of the constraints, but still ran into issues.

In general, every constraint should be scaled however you should determine the correct approach to use for each constraint. I.e. you need to understand each constraint and what is the appropriate way to scale it. In this case, using inverse maximum is sufficient for all the constraints, but more complex models may require a mix of scaling schemes.

Also, be careful not to conflate poor scaling with poorly posed constraints. Poorly posed constraints are inherently poorly scaled and cannot be fixed by just applying a scaling factor - the latest diagnostics tools have some new checks for poorly posed constraints.

@andrewlee94 just to be sure, can you elaborate on which new checks you are referring to?

display_constraints_with_mismatched_terms and display_constraints_with_canceling_terms, along with display_problematic_constraint_terms for details on what is wrong in each constraint. These are shown as Cautions in the numerical issues report.

Just as an FYI, none of the BSM2 constraints are poorly posed. But I will definitely re-visit the constraint scaling for BSM2 when I get the time.

Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>

adam-a-a · 2025-02-04T17:33:36Z

watertap/unit_models/nanofiltration_ZO.py

@@ -46,6 +47,12 @@
 _log = idaeslog.getLogger(__name__)


+@deprecated(
+    "The NanofiltrationZO model has been deprecated in favor of the Nanofiltration0D model. "
+    "The Nanofiltation0D model has most of the capabilities of the NanofiltrationZO model "


Thanks for this! You say the new model has "most of the capabilities". Can you remind me what the new model might not have that the deprecated version does? It's been a while since I reviewed this PR.

Off the top of my head, it lacks the water/solvent flux and recovery calculations. That might be all however.

If we are to replace the deprecated version with this one eventually (and remove the deprecated later), we should probably preserve solvent flux, recovery, and anything else that might be missing.

The first catch I see there is that to have solvent flux requires additional user input that is not required for the simplest version (at a minimum, membrane area). So, it does open the question of whether we want to have that as a default inclusion, or if it should be an optional add-on.

Also, I am not sure how much time I will have to work on this right now. Someone else is welcome to take over if we decide it is important to add the other components.

I think that we should include flux, area, and other basic "essentials" that are in the existing NF model.

We can have someone else ensure those additions are made and then merge this in.

adam-a-a · 2025-02-04T17:42:35Z

watertap/unit_models/nanofiltration_0D.py

+        self.add_port("permeate", self.properties_permeate, doc="Permeate Port")
+
+        # NF separation variables
+        self.solvent_recovery = Var(self.flowsheet().time, initialize=0.8)


This should have a doc string. Also, recovery_vol would be more in line with our naming convention (although we have used recovery_vol_phase indexed by Liq; saving effort of having to add an unnecessary index). Unless this is meant to be mass recovery and not volumetric, in which case recovery_mass_solvent is more in line with existing naming conventions. Can also consider recovery_mass_comp which would give mass recovery of solvent and solutes.

So, I agree the order of should be reversed to match standards but the basis is a little more vague. As a recovery fraction for a species as a whole, mass, mole and volume basis are equivalent, so I am not sure we should qualify it. Thus, my suggestion would be recovery_solvent.

OK-- that works.

adam-a-a · 2025-02-04T17:46:48Z

watertap/unit_models/nanofiltration_0D.py

+        if self.config.energy_balance_type == EnergyBalanceType.isothermal:
+
+            @self.Constraint(
+                self.flowsheet().time, doc="Retentate temperature equality"
+            )
+            def retentate_temperature_equality(b, t):
+                return (
+                    b.properties_in[t].temperature
+                    == b.properties_retentate[t].temperature
+                )
+
+            @self.Constraint(self.flowsheet().time, doc="Permeate temperature equality")
+            def permeate_temperature_equality(b, t):
+                return (
+                    b.properties_in[t].temperature
+                    == b.properties_permeate[t].temperature
+                )


Any reason not to just apply the modified control volume? I was assuming that it was written in a way that one wouldn't need to write out the equality constraints (but been a while since last reviewed new control volume).

Mainly because this model does not use any control volumes (but does require the EnergyBalanceType.isothermal option in the Enum). In short, as this is basically a specialised separator a control volume does not add much as we would need to write at least half the constraints manually anyway.

ksbeattie · 2025-02-20T21:26:31Z

@andrewlee94 any update on this?

andrewlee94 · 2025-02-20T21:28:35Z

@ksbeattie I am waiting on a response to one of @adam-a-a's comment above.

adam-a-a · 2025-03-06T21:45:41Z

setup.py

@@ -67,7 +67,7 @@
    python_requires=">=3.9",
    install_requires=[
        # primary requirements for unit and property models
-        "idaes-pse >=2.7.0,<2.8.0rc0",
+        "idaes-pse @ https://github.com/watertap-org/idaes-pse/archive/2.7.0.dev.watertap.2025.01.21.zip",


Need to update this

@lbianchi-lbl reminder to check this out and ensure update to latest idaes release works here

kurbansitterley · 2025-03-06T22:30:24Z

@adam-a-a @lbianchi-lbl I could not duplicate the macOS failure locally.

All tests pass for me locally except TestNanofiltration0DInitializer::test_initialize where I get:

FAILED watertap/unit_models/tests/test_nanofiltration_0D.py::TestNanofiltration0DInitializer::test_initialize - DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses

Then, when I try to duplicate that error outside of pytest, I get:

2025-03-06 15:23:38 [INFO] idaes.init.fs.unit: Initialization Completed, optimal - <undefined>

Prior to running this, I re-installed my environment with the setup.py on this PR.

Probably relevant information:

MacBook Pro, 14-inch, 2021
Apple M1 Pro
macOS 14.7.1

@lbianchi-lbl do you know if the test is run with an Intel chip (vs. M1)?

🤷‍♂️

lbianchi-lbl · 2025-03-06T22:48:36Z

@kurbansitterley the check is indeed running on macOS Intel, specifically using IPOPT from conda-forge (as opposed to the HSL-enabled build available via idaes get-extensions. So it's not unexpected that this test might exhibit different behavior on an Apple Silicon machine (such as yours); however, it's still informative since we don't yet have Apple Silicon running in CI. If anything, this is a further confirmation that we should retire this CI check for good and replace with Apple Silicon CI jobs as per #1306.

kurbansitterley · 2025-03-06T22:57:01Z

@kurbansitterley the check is indeed running on macOS Intel, specifically using IPOPT from conda-forge (as opposed to the HSL-enabled build available via idaes get-extensions. So it's not unexpected that this test might exhibit different behavior on an Apple Silicon machine (such as yours); however, it's still informative since we don't yet have Apple Silicon running in CI. If anything, this is a further confirmation that we should retire this CI check for good and replace with Apple Silicon CI jobs as per #1306.

FWIW I don't know of any Mac-using WaterTAP devs with an Intel processor. So I am not sure if we are able to troubleshoot this issue.

M1s came out in 2020, one year after Python 3.8 was released, which we also stopped testing against, sooo... I support the retirement of this experimental test.

andrewlee94 added 5 commits December 2, 2024 10:27

Cherry picking files from other branch

10d77c9

Testing NF0D

2df7d0b

More testing

348fc0e

Finishing tests of NF0D

95c59c4

MCAS tests and black

c018b17

andrewlee94 self-assigned this Dec 2, 2024

andrewlee94 added enhancement New feature or request Priority:Normal Normal Priority Issue or PR oli labels Dec 2, 2024

adam-a-a reviewed Dec 2, 2024

View reviewed changes

andrewlee94 added 6 commits December 2, 2024 16:15

Docs and linting

de2d18a

Running black

62cd994

Fixing duplicae label in docs

46e9d1c

Merge branch 'main' into nf0d

028a598

Linting

d65bae2

Merge branch 'nf0d' of https://github.com/andrewlee94/watertap into nf0d

5121d83

adam-a-a requested changes Dec 11, 2024

View reviewed changes

andrewlee94 and others added 9 commits January 2, 2025 09:06

Standardize momentum balance args

82d0ec5

Switching to rejection

97c0a70

Working around MCAS issue

f7039e7

Merge branch 'main' into nf0d

ec88cae

Fixing energy balance config arg

7a73ebb

Pylint

923619f

Bumping IDAES tag

181614f

Merge branch 'main' into nf0d

57b1fa7

Deprecation warning fo NF ZO

2741d0e

ksbeattie requested review from MarcusHolly and TimBartholomew January 23, 2025 21:34

MarcusHolly reviewed Jan 30, 2025

View reviewed changes

Update docs/technical_reference/unit_models/nanofiltration_0D.rst

dc632ed

Co-authored-by: MarcusHolly <96305519+MarcusHolly@users.noreply.github.com>

adam-a-a reviewed Feb 4, 2025

View reviewed changes

Rename solvent recovery variable

11b7584

adam-a-a reviewed Mar 6, 2025

View reviewed changes

MarcusHolly mentioned this pull request Mar 6, 2025

Implementing IDAES Scaler Objects into BSM2 #1544

Open

12 tasks

		@declare_process_block_class("Nanofiltration0D")
		class Nanofiltration0DData(UnitModelBlockData):

	Zero order nanofiltration model.
	Zero dimensional nanofiltration model.

		"Solute recovery", ":math:`R_j`", "solute_recovery", [j], ":math:`\text{dimensionless}`"
		"Multivalent ion recovery", ":math:`R_{multi}`", "multivalent_recovery", None, ":math:`\text{dimensionless}`"

Simple OD Nanofiltration model #1536

Are you sure you want to change the base?

Simple OD Nanofiltration model #1536

Conversation

andrewlee94 commented Dec 2, 2024

Fixes/Resolves: None

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-a-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewlee94 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewlee94 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-a-a Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksbeattie commented Feb 20, 2025

andrewlee94 commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurbansitterley commented Mar 6, 2025

lbianchi-lbl commented Mar 6, 2025

kurbansitterley commented Mar 6, 2025

andrewlee94 Dec 11, 2024 •

edited

Loading

andrewlee94 Dec 11, 2024 •

edited

Loading

adam-a-a Dec 11, 2024 •

edited

Loading