Base class for handling the spacer correlations and process block cla…

[charansamineni]

…ss to create a RO 1D unit model with user choice for spacer type, sherwood correlation, and pressure drop calculation. Original constraints are not deleted just deactivated

Fixes/Resolves:

(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)

Summary/Motivation:

Changes proposed in this PR:

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.

[ksbeattie]

@charansamineni might you be able to provide some tests and documentation updates for this?

[charansamineni]

@ksbeattie, I am working on getting it ready. I have to add some more functionality based on feedback from reviewers.

[adam-a-a]

Thank you for putting up these proposed changes, and congratulations on putting up your first PR here! I have long wanted to incorporate more detailed correlations related to spacer geometry and am glad that you've taken the step to do so.

There is a bit of work to do here before we can merge these changes. After my first pass, here are my overall, initial suggestions:

1. Add a test file for ro_1D_custom_spacer. Here, you can test different configuration setups for RO, which would show reviewers more clearly how the changes would channel through to the user. Moreover, this would give you the chance to identify low-hanging bugs in the existing code. Perhaps you shouldn't add every test under the sun in relation to the correlations in custom_spacer_base because, if we do keep the separate class, I would think there would be a dedicated test file for that class. That said, it won't hurt to do so minimal level of testing and move tests later. From here, we should do another pass of reviews.
2. After that, once you have some basic tests that probe the various ways of configuring RO1D, now you can switch gears and try merging changes from ro_1D_custom_spacer directly into the existing classes for RO and/or membrane channel. You would also add your new tests for this custom RO1d to the existing test file for RO1d. Many things will break, and all that breakage will need repair until tests pass.

Try doing step 1, and let's revisit the custom_spacer_base class after that.

[adam-a-a]

The first thought that comes to mind is that this should not be a separate unit model. Although it could be, this would add redundancy that we typically try to avoid. Have you tried bringing your changes into the existing RO1D model? If we were to implement these changes, we'd likely want to incorporate into the membrane_channel and RO classes, where relevant. This makes the implementation a little trickier but is ultimately what should be merged into the main codebase.

[adam-a-a]

This would be unnecessary once incorporating into our existing RO unit, but I do like the idea of logging what the user ultimately defined in the RO model configuration! No need to pursue that in this PR, but given the increasing complexity of config options, it might not be a bad idea for the user to get a snapshot of what config settings they ultimately are working with upon instantiation, since much of this can be opaque to new and experienced users alike.

[adam-a-a]

This method is going to require quite a bit of testing to ensure that intended combinations of config options work the way they should, and unintended, incorrect combinations actually raise the exception that you would expect.

[adam-a-a]

Initially, I would think that the N_sh constraint should keep the original name rather creating a new constraint name, for the purpose of maintaining symmetry in the model when possible.

[adam-a-a]

I would prefer this not to be here. Instead, it'd be great if we just kept eq_N_Sh_comp. This works as a bandaid for doing analysis locally, but in terms of merging to main and documenting model additions, having the one original constraint makes more sense.

[adam-a-a]

same thing here---rather deactivate the original constraint and activate a new one, we should try to just use the original constraint and make appropriate changes to do so with overwriting the constraint, which Pyomo will throw up a warning for.

[adam-a-a]

These are intended for the feed_side of RO, which indicates that this should be in membrane_channel or the RO class. Can any of the correlations be applied to the permeate side, say, in OARO? If yes, then putting in one of the membrane_channel cases might make sense. If limited to the feed side for RO, then I wonder whether this should live here. In essence, custom_space_base hinges upon RO, but the fact that its introduced as a separate base class could indicate otherwise. Does it make more sense to not have this separate base class? I say this only after a relatively quick scan--it could be that having this separate class is beneficial. Will be able to tell more after testing is introduced.

[adam-a-a]

FYI, we normally import constants like pi from idaes.core.util.constants. In this case, idaes just uses the same math import for pi anyway, so doesn't matter here.

[adam-a-a]

Should it always be the case that spacer height is 2*D? If not, should we add a mutable Param initialized to 2 instead? Also, are we introducing redundant constraints by doing this since spacer height=channel height?

[adam-a-a]

I guess the question is--is there ever a case when spacer height does not equal channel height? If there is no such exception, I would suggest only maintaining channel_height. And if you really are tied to referring to spacer height, you could always create an Expression, spacer_height, which would just point to channel_height

[adam-a-a]

Given the breadth of additions in this PR, we'll definitely want to add documentation to readthedocs to capture all the additions. That can come at the end of this though, once satisfied with the core code changes.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 300 lines in your changes missing coverage. Please review.

Project coverage is 92.87%. Comparing base (e93578f) to head (af722d4).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
watertap/core/custom_spacer_base.py	0.00%	197 Missing ⚠️
watertap/unit_models/ro_1D_custom_spacer.py	0.00%	103 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1496      +/-   ##
==========================================
- Coverage   93.78%   92.87%   -0.92%     
==========================================
  Files         282      284       +2     
  Lines       29996    30697     +701     
==========================================
+ Hits        28133    28510     +377     
- Misses       1863     2187     +324     

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

[ksbeattie]

@charansamineni any news on this?