Ion pp mod

[lbibl]

Fixes/Addresses:

• Added new vars including electrical_mobility_comp and electrical_conductivity_phase and related methods.
• Modified the initialization method to set values for quantities calculated from state vars.
• Modified scaling methods as needed.
• Modified default values for some vars as needed.
• Modified the test file to reflect changes in the property package.

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.

[aladshaw3]

@lbibl @adam-a-a Looks like the changes are causing initialization failures in the nanofiltration model. More testing and modifications will be needed (most likely with the scaling of the effected models).

[adam-a-a]

@lbibl @adam-a-a Looks like the changes are causing initialization failures in the nanofiltration model. More testing and modifications will be needed (most likely with the scaling of the effected models).

@aladshaw3 - Yeah, which was what I was worried about. The NF certainly needs refinement, but that's something that I need to work on for the quarter. Maybe we'll get by with a subtle adjustment so that tests pass for this PR. Otherwise, we might want to consider creating a separate dev branch for this (or some other solution) so that the refinement of the NF model doesn't impede your workflow with ED.

[bknueven]

@lbibl @adam-a-a Looks like the changes are causing initialization failures in the nanofiltration model. More testing and modifications will be needed (most likely with the scaling of the effected models).

@aladshaw3 - Yeah, which was what I was worried about. The NF certainly needs refinement, but that's something that I need to work on for the quarter. Maybe we'll get by with a subtle adjustment so that tests pass for this PR. Otherwise, we might want to consider creating a separate dev branch for this (or some other solution) so that the refinement of the NF model doesn't impede your workflow with ED.

Given there are relatively minor changes here, shouldn't we question if the scaling changes made in this PR are correct? I think it would be better to make the minimal changes required for this feature first and then maybe in a separate PR make unrelated changes to the way this property package is scaled, being sure to test it against every place its used.

[adam-a-a]

@lbibl @adam-a-a Looks like the changes are causing initialization failures in the nanofiltration model. More testing and modifications will be needed (most likely with the scaling of the effected models).

@aladshaw3 - Yeah, which was what I was worried about. The NF certainly needs refinement, but that's something that I need to work on for the quarter. Maybe we'll get by with a subtle adjustment so that tests pass for this PR. Otherwise, we might want to consider creating a separate dev branch for this (or some other solution) so that the refinement of the NF model doesn't impede your workflow with ED.

Given there are relatively minor changes here, shouldn't we question if the scaling changes made in this PR are correct? I think it would be better to make the minimal changes required for this feature first and then maybe in a separate PR make unrelated changes to the way this property package is scaled, being sure to test it against every place its used.

That makes sense. However, I was able to get the NF test working just now locally. I just had to reduce the default scaling factors that I had set for two ions and adjust some regression tests (for a variable that has already been problematic in NF model). I just can't push the changes because @lbibl needed to enable permission to do so.

[aladshaw3]

@lbibl @adam-a-a Looks like the changes are causing initialization failures in the nanofiltration model. More testing and modifications will be needed (most likely with the scaling of the effected models).

@aladshaw3 - Yeah, which was what I was worried about. The NF certainly needs refinement, but that's something that I need to work on for the quarter. Maybe we'll get by with a subtle adjustment so that tests pass for this PR. Otherwise, we might want to consider creating a separate dev branch for this (or some other solution) so that the refinement of the NF model doesn't impede your workflow with ED.

Given there are relatively minor changes here, shouldn't we question if the scaling changes made in this PR are correct? I think it would be better to make the minimal changes required for this feature first and then maybe in a separate PR make unrelated changes to the way this property package is scaled, being sure to test it against every place its used.

That makes sense. However, I was able to get the NF test working just now locally. I just had to reduce the default scaling factors that I had set for two ions and adjust some regression tests (for a variable that has already been problematic in NF model). I just can't push the changes because @lbibl needed to enable permission to do so.

Are the changes significant? Can you just use the GitHub interface to commit those changes directly in this PR?

[bknueven]

@lbibl you need to check this box so @adam-a-a can commit his changes to this PR. In general, there's no real reason to leave this unchecked.

[aladshaw3]

@lbibl You will also need to run black on both these files to pass the format check. You can do this in terminal as:

black {file_name}

[lbibl]

Hi all, thanks for your messages. Before I opened the PR, pytest test_nanofiltration_DSPMDE_0D.py was able to pass on my local but did show 4 "xfailed " . So I wonder if my arm64 env still missed something and will look into that. Meanwhile, if @adam-a-a can make reasonable/easy changes on the nf unit model as a fix, that'll be great too. I have checked that box to allow changes from maintainers and will have it checked for the future. Also just blacked the two files to pass the formatting checks and the commit was pushed. Sorry for the negligence in my first PR. @aladshaw3 @bknueven @adam-a-a

[aladshaw3]

@adam-a-a I think there is something fundamentally wrong with the scaling in the nanofiltration model (since we are getting different model results / failing tests on different python versions and OSs). I can have a look at it more today, but we may need to just disable the nanofiltration model and revisit it later.

[aladshaw3]

@adam-a-a I have checked the log during initialization for the nanofiltration model. The results indicate that the unit model itself (nanofiltration_DSPMDE) is causing the issue. The properties are properly scaled and solve well during initialization, but the unit model never solves. (see attached).

nanofiltration_DSPMDE_initialization_log.txt

This tells me that any issue with failing tests is caused by the nanofiltration model and not the changes made in the property package. I think we may want to consider disabling the nanofiltration model tests and revisiting at another time so that this PR can get done soon.

[aladshaw3]

For some reason, there is a documentation failure now. It is from the newly merged docs (#500). That PR passed all its checks, so I am not sure why it is failing now.

FailingDocLog-python3_8.txt

[adam-a-a]

@adam-a-a I have checked the log during initialization for the nanofiltration model. The results indicate that the unit model itself (nanofiltration_DSPMDE) is causing the issue. The properties are properly scaled and solve well during initialization, but the unit model never solves. (see attached).

nanofiltration_DSPMDE_initialization_log.txt

This tells me that any issue with failing tests is caused by the nanofiltration model and not the changes made in the property package. I think we may want to consider disabling the nanofiltration model tests and revisiting at another time so that this PR can get done soon.

@aladshaw3 Yes, I'm aware of the issue and realized that some tests pass on Windows while Linux tests fail. The NF model requires a bunch of refinement (outlined in issue #464 ).

At first, I was alarmed by your suggestion, until I realized that you only intended to skip specific tests rather than the whole set of tests for NF DSPM. So-- I think I'm OK with that.

[codecov]

Codecov Report

Merging #508 (25aa85b) into main (b9a7d79) will decrease coverage by 0.53%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##             main     #508      +/-   ##
==========================================
- Coverage   94.72%   94.19%   -0.54%     
==========================================
  Files         171      171              
  Lines       14018    14057      +39     
==========================================
- Hits        13279    13241      -38     
- Misses        739      816      +77     

Impacted Files	Coverage Δ
watertap/property_models/ion_DSPMDE_prop_pack.py	91.00% <97.82%> (+0.60%)	⬆️
watertap/unit_models/nanofiltration_DSPMDE_0D.py	80.07% <0.00%> (-14.71%)	⬇️

---

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 b9a7d79...25aa85b. Read the comment docs.

[adam-a-a]

For some reason, there is a documentation failure now. It is from the newly merged docs (#500). That PR passed all its checks, so I am not sure why it is failing now.

FailingDocLog-python3_8.txt

I noticed this issue elsewhere too, but I think I managed to make that go away by rerunning the failed test. In any case, we probably want to resolve this sooner rather than later (i.e., updating IDAES tag as you mentioned in issue #503 ).

[aladshaw3]

@adam-a-a I have checked the log during initialization for the nanofiltration model. The results indicate that the unit model itself (nanofiltration_DSPMDE) is causing the issue. The properties are properly scaled and solve well during initialization, but the unit model never solves. (see attached).
nanofiltration_DSPMDE_initialization_log.txt
This tells me that any issue with failing tests is caused by the nanofiltration model and not the changes made in the property package. I think we may want to consider disabling the nanofiltration model tests and revisiting at another time so that this PR can get done soon.

@aladshaw3 Yes, I'm aware of the issue and realized that some tests pass on Windows while Linux tests fail. The NF model requires a bunch of refinement (outlined in issue #464 ).

At first, I was alarmed by your suggestion, until I realized that you only intended to skip specific tests rather than the whole set of tests for NF DSPM. So-- I think I'm OK with that.

As another point of comparison, I ran the nanofiltration model before the changes made in this PR and here is that log.
nanofiltration_DSPMDE_initialization_log_pre_changes.txt

Comparing to the new log (with the changes proposed in this PR), you can see that the initialization of the properties section has improved because of the changes made by @lbibl.

[aladshaw3]

Correct me if I am wrong, but I thought we had discussed changing the 'bounds' on many of these variables because they didn't go low enough. Especially, for things like mole fractions and concentrations, we will need to allow for smaller concentration values if we want to consider very dilute systems.

Same to be said of the variables on lines (784 - dens_solvent, 854 - conc_mass, 912 - molality, and 960 - act_coeff). I don't think the bounds on these variables are correct or accurate. For example, the activity coefficient can go greater than 1 and mass concentration should be allowed to go lower than 1e-8.

[aladshaw3]

This looks good to go. We are skipping some nanofiltration_DSPMDE tests for now. That model needs to be refined in a separate PR.

[adam-a-a]

LGTM

[lbibl]

I am correcting a few minor issues per comments above. Please wait for at least another commit from me before merging.

[lbibl]

In the latest commit (25aa85b) I fixed minor issues and added initialization method for a few other vars. Test for ion property package passed. Maybe @adam-a-a or @aladshaw3 may try if the added initialization for osmosis pressure improves the nanofiltation model. This would be good to go if no other input at this stage.

[aladshaw3]

LGTM

[adam-a-a]

LGTM