Multi-effect crystallizer unit model #121
Conversation
|
@Zhuoran29 can you run black so we can see the results of the GH tests please? |
Sure, it seems the cost package is not adjusted well and I'll look into it. |
| def test_initialize(self, Crystallizer_frame): | ||
| # Add costing function, then initialize | ||
| m = Crystallizer_frame | ||
| # m.fs.costing = WaterTAPCosting() |
There was a problem hiding this comment.
@Zhuoran29 just skimming this from my phone, but maybe the failed tests are due to the costing lines being commented out here.
There was a problem hiding this comment.
In any case, I’d recommend just having a separate test_costing test rather than bundle it up in the initialization test.
There was a problem hiding this comment.
Now the costing package is rebuilt in REFLO and tested separately.
…to crystallizers
…-reflo into crystallizers
|
I added the costing package from the WaterTAP repo and a test file for the flowsheet, where both simulation and optimization modes were tested. |
|
Hey @kurbansitterley @adam-a-a , forgot to tag you when I updated this PR. |
| opt = get_solver(solver, optarg) | ||
| for n, eff in self.effects.items(): | ||
| if n == 1: | ||
| assert degrees_of_freedom(eff.effect) == 0 |
There was a problem hiding this comment.
This should raise an exception if DOF do not equal 0
| return m | ||
|
|
||
|
|
||
| class TestMultiEffectCrystallizer_4Effects: |
There was a problem hiding this comment.
So 4 effects is the default, and you test for 4 effects and 3 effects. What happens if the user tries entering 1 effect? Similarly, what about 2 effects? I went scanned through the core code quickly, so I know I missed this. I just wonder if we should just add two quick, shortened versions of these tests just to ensure that we get what we'd expect from entering 1 effect or 2 effects. For example, in 2 effects, I might expect some special handling of the first and last effect, hence interest in this case. For 1 effect, I would expect the code to handle this case or throw an exception if the multieffect model is not setup to handle specification of 1 effect.
There was a problem hiding this comment.
addressed in e0b5b44.
Added test for 2 effects.
If the user tries to use MultiEffectCrystallizer with a single effect, it will raise ConfigurationError and they will be directed to use the CrystallizerEffect model.
There was a problem hiding this comment.
Nice. One thought I have--say you wanted to run optimization cases across a range of conditions and a range of number of effects, including a single effect. You'd need to add some logic in to such a script to toggle between the CrystallizerEffect model for the case where a single effect is optimized, and then toggle to the MultiEffectCrystallizer model for all other cases.
All that just to say, it might be useful to actually allow for a single effect via MultiEffectCrystallizer too. Definitely not something to consider for this PR--just food for thought.
There was a problem hiding this comment.
Thinking about this now, I don't think it would be very hard to implement, maybe a little clunky, but I do like the idea of having all the functionality for a unit model within a single model rather than directing the user to use a different unit model. A consideration for the future.
|
My other general comment is we definitely want to document the single and multieffect models thoroughly--though that can happen in a subsequent PR down the line and shouldn't hold this up. |
…-reflo into pr/Zhuoran29/121
Co-authored-by: Adam Atia <[email protected]>
|
I ran a few cases and the results are looking good to me! Also I added a test for an optimization scenario in case it's helpful to the case study. If the additions look good to you then I think it's good to go, but please feel free to not include it. @kurbansitterley |
This PR adds:
CrystallizerEffectunit modelMultiEffectCrystallizerunit model that is comprised of nCrystallizerEffectunit models connected via constraintsMultiEffectCrystallizerandCrystallizerEffectwatertap==1.0.0rc0in setup.pyTODO:
CrystallizerEffectflow_vol_phase['Vap']is the proper flow to cost for steam