Dewatering and thickener unit models BSM2

[agarciadiego]

Summary/Motivation:

Unit model necessary for BSM2 flowsheet. Addresses point in Issue #934

Changes proposed in this PR:

• Include dewatering unit model
• Include thickener unit model

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]

Codecov Report

Attention: Patch coverage is 97.22222% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.82%. Comparing base (a4fed99) to head (84585e5).
Report is 335 commits behind head on main.

Files with missing lines	Patch %	Lines
watertap/unit_models/thickener.py	96.29%	2 Missing ⚠️
watertap/unit_models/dewatering.py	98.07%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #1010    +/-   ##
========================================
  Coverage   95.81%   95.82%            
========================================
  Files         296      298     +2     
  Lines       28475    28583   +108     
========================================
+ Hits        27284    27389   +105     
- Misses       1191     1194     +3     

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

[MarcusHolly]

LGTM - I will add the documentation under a separate PR.

[adam-a-a]

I would shoot for more elaborate naming schemes. My guess without referring back to the literature is that you went with matching symbols in the literature. While that seems reasonable for the popular ASM and ADM models, I wonder if it might be more useful to offer up our own names in these unit cases for improved clarity. This is a broad comment regarding any parameter/variable names, when I compare those names with the associated doc string and question their alignment.

[agarciadiego]

I think if we are following ASM and ADM convention for names of our property, reaction package, etc. following naming convention from BSM2 paper would avoid confusion for users specially if they are following the paper we added as references.

[adam-a-a]

Fair enough - the documentation will probably help things.

[adam-a-a]

So, for both units, I'm aware that you didn't add initialize_build and calcualte_scaling_factors methods, likely because the separator handles those. I still wonder though if we should include them here anyway, even if, for example, we just have loggers setup for initialization and run super().calculate_scaling_factors(). @andrewlee94 curious to hear your thoughts.

[andrewlee94]

If you inherit from another model or base class, then any methods you do not overload will use that of the parent. For initialization this might be OK as long as the additions in the new model are relatively simple. However, for scaling, the new model is adding new variables and constraints (otherwise it wouldn't be a new model), and the parent's scaling methods won't touch these leaving part of the model unscaled.

On a related note, be aware that IDAES plans to rebuild both initialization and scaling this year. The new scaling API is already in place if you want to start looking at it: https://idaes-pse.readthedocs.io/en/latest/reference_guides/initialization/index.html

[adam-a-a]

Right, I figured we could technically get away with it since the Separator's initialization and calculate_scaling_factors are essentially being used here, but that it might just be good practice and make for better readability/consistency between models for [new] developers. Furthermore, there are probably some unscaled vars that are added here but simple enough that additional scaling doesn't seem necessary at this point. I think we could put a pin in it for now and revisit if issues arise when analyzing the BSM2 flowsheet later.

And thanks for the note on the new scaling API.

[adam-a-a]

There should typically be a test to check for badly scaled variables here. I checked, and a few will crop up. We can be lenient about it and make a note to revisit in our master issue, considering the whole effort here for the BSM2 flowsheet, or you could add the typical test to check for badly scaled vars and eliminate any that exist by adding scaling. Though the badly scaled vars are probably attributed to zero components, it's hard to say whether this will end up impacting model stability at the flowsheet level later on.

[adam-a-a]

Actually, through our off-GH discussion, you made it clear that the badly scaled vars stem from the lack of scaling on the ASM1 property model, which we have noted in our master issue (#934)

[adam-a-a]

LGTM