Improving the NF0D model

[adam-a-a]

Fixes/Addresses:

Summary/Motivation:

This PR refines the 0D nanofiltration model by adding the same structure that the RO model has and enabling detailed config options (concentration polarization, pressure drop calculations).

A current limitation of this model is that it only accepts one solute, and we want to aim for a model that can handle multi-ionic solutions. That is a work in progress and will be reserved for an upcoming PR.

Changes proposed in this PR:

• Add same structure to NF as RO
• Add some minimal testing for NF (minimal only because I want to leapfrog to more detailed model for potential use in full treatment train)

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.

[codecov-commenter]

Codecov Report

Merging #200 (031caab) into main (4a9f5d0) will decrease coverage by 2.78%.
The diff coverage is 66.93%.

@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
- Coverage   90.46%   87.68%   -2.79%     
==========================================
  Files          23       23              
  Lines        3997     4424     +427     
==========================================
+ Hits         3616     3879     +263     
- Misses        381      545     +164     

Impacted Files	Coverage Δ
proteuslib/unit_models/nanofiltration_0D.py	71.29% <66.93%> (-18.75%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a9f5d0...031caab. Read the comment docs.

[aladshaw3]

Why is the upper bound 1 - 1e-6? Is there some significance to it? Can it not just be 1?

[adam-a-a]

This was taken from previous code. I think one reason is because in certain places, there could be implementation of 1- self.recovery_vol_phase. That could lead to division by zero.

Also, you can't realistically have a recovery rate of 1, so this is just meant to set an upper limit that approaches 1 (even that is not really practical in reality though).

[andrewlee94]

In these cases I would personally use 1 for two reasons.

1. It represents the limit of recovery/removal. which is one interpretation of a bound.
2. Doing 1-eps applies an arbitrary limit on the maximum removal/recovery. It probably does not matter in practice, but it just feels wrong to me.

On the matter of the potential division by zero, that would be better addressed by trying to formulate the constraint such that a division by zero cannot occur. Even with an arbitrary bound on the approach to 0, you still need to deal with the non-linearities of approaching a singularity.

[adam-a-a]

I don't see any other reason not to change this upper bound to 1 as long as it doesn't cause any unexpected issues. The division by zero issue was just my guess. @TimBartholomew do you recall the significance of setting this upper bound to 1 - 1e-6?

[aladshaw3]

Same as previous comment.

[adam-a-a]

The same idea applies here as in my response to the previous comment, except the variable in this case is solute rejection.

[aladshaw3]

In general, we should move away from 'user-scaling' for the solver. However, if this is the only way to get this to solve for now, then I guess we need to keep it.

[adam-a-a]

Certainly- thank you for discovering the discrepancy yesterday as this was something we were overlooking. @bknueven made some nice progress with adjusting scaling factors by probing extreme Jacobian entries, but this will likely be reserved for a subsequent PR , once we confirm the logic/stability of newly adopted scaling factors.

[aladshaw3]

LGTM

[aladshaw3]

LGTM

[TimBartholomew]

I'm not sure we will want to merge this into ProteusLib. Its not clear what the use case is, i.e. who would use this model, or if we can verify its implementation, i.e. no literature to compare against. We also want to reduce the amount of copied code and this PR primarily copies code from RO and moves it into NF.

These criticisms are for the NF in general. I'm think we may want to remove this one from the repo and only include the experimental zero order model.

[adam-a-a]

I'm not sure we will want to merge this into ProteusLib. Its not clear what the use case is, i.e. who would use this model, or if we can verify its implementation, i.e. no literature to compare against. We also want to reduce the amount of copied code and this PR primarily copies code from RO and moves it into NF.

These criticisms are for the NF in general. I'm think we may want to remove this one from the repo and only include the experimental zero order model.

Although the Kedem-Katchalsky model may not be the model of choice, especially with regard to our full treatment train use case, I see no harm in including it. After all, it is a widely used model despite its limitations. There may be particular use cases for users who are looking to implement a simple NF model, e.g., for the primary treatment process, focusing on a single target component for removal. Perhaps experimentalists have developed a new membrane and used the Kedem-Katchalsky model to obtain phenomenological coefficients from their data; they could use such data in this model to simulate their new membrane's performance.

With regard to the overlap with RO model code, I assumed this could be resolved later, once we begin refactoring the RO models and create a base model that could account for generic membrane module variables and constraints. Such a base class could also serve this Kedem-Katchalsky (and other) NF model(s). Although the zero-order experimental model will be of use to us for the full treatment train, as well as for others who seek a simplified approach to modeling multi-ion removal from seawater, its usage would be fairly limited. Meanwhile the Kedem-Katchalsky model, though also limited in its own ways, could facilitate more generic use cases. All that to say that I think we should merge this instead of scrapping it (perhaps rename it something like nanofiltration_KKmodel_0D) because we can have variations in modeling approaches.

[bknueven]

I have no opinion on @TimBartholomew's first point (whether this model should be part of ProteusLib), but I agree with @adam-a-a on the second. If we want to go forward with this NF0D model I could begin removing redundant code as part of this PR -- that said, it's probably better handled separately.

[adam-a-a]

This PR is on the backburner for now and is of lower priority than our full treatment train effort (#209). Should verify whether reflection coefficients and permeability values for salts would still be useful (as opposed to coefficients for ions). However with the Kedem-Spiegler modifications, we would have a relationship for salt rejection as a function of the reflection coefficient, and salt rejection values for apparent species is not uncommon. Another note: need to add more testing as well as modification of the documentation. @TimBartholomew tagging you so that you see this in case you have anything to add.

[adam-a-a]

Converting this to a draft so that we can convert this to a Kedem-Katchalsky-Spiegler model that can handle multicomponent solutions (may have found a couple of good examples in the literature).

[adam-a-a]

Closing this draft PR since we will be focusing on implementing the Donnan steric pore model w/dielectric exclusion soon.