Modular properties fixes and enhancements #1656
Conversation
| """ | ||
| try: | ||
| if hasattr(model, "fix_initialization_states"): | ||
| model.fix_initialization_states() | ||
| except AttributeError: | ||
| else: | ||
| _log.info_high( | ||
| f"Model {model.name} does not have a fix_initialization_states method - attempting to continue." | ||
| ) |
There was a problem hiding this comment.
This change revealed that there was an error in the legacy Cubic EoS initialization---the fix_initialization_states method was trying to deactivate sum_mol_frac_out, but that constraint doesn't appear to exist (at least, not with that name), which was raising an AttributeError that was swallowed in the try/except block. That illustrates why this change was necessary.
Since we said that we were going to remove the legacy CubicEoS in the May release, the straightforward solution is to just remove it now.
| if not log: | ||
| try: | ||
| mthd = c_arg.return_expression | ||
| except AttributeError: | ||
| mthd = c_arg | ||
| else: | ||
| mthd = c_arg.return_log_expression |
There was a problem hiding this comment.
Do we want to require the user to add a return_log_expression function to their implementation of Antoine's law?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1656 +/- ##
==========================================
+ Coverage 76.95% 77.06% +0.10%
==========================================
Files 397 395 -2
Lines 63580 62761 -819
Branches 10367 10233 -134
==========================================
- Hits 48929 48366 -563
+ Misses 12213 11985 -228
+ Partials 2438 2410 -28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The test failure in Examples requires a fix to be pushed to the Examples repo. |
| scaler_block = [blk for blk in model.values()][0] | ||
|
|
||
| if hasattr(scaler_block, "inherent_equilibrium_constraint") and ( | ||
| not scaler_block.params._electrolyte # TODO why do we need this check? |
There was a problem hiding this comment.
is TODO necessary? I beleive it creates confusion on who we are talking to
There was a problem hiding this comment.
Would you like me to remove the TODO or the entire comment?
| or scaler_block.params.config.state_components == StateIndex.true | ||
| ): | ||
| init_log.debug( | ||
| "Cannot converge inherent reaction constraints " |
There was a problem hiding this comment.
This warning may be confusing. When we say "they need to be solved at the control volume level," the user may not know if that is something they must now do.
| or "mole_frac_phase_comp" in k.define_state_vars() | ||
| ): | ||
| k.equilibrium_constraint.deactivate() | ||
| # TODO Inherent reactions with a true component basis will fail here too |
There was a problem hiding this comment.
Do not understant TODO message
| for idx in self.fug_phase_comp: | ||
| sf_x = iscale.get_scaling_factor( | ||
| self.mole_frac_phase_comp[idx], | ||
| default=1e3, # I'd prefer 10, but this is consistent with existing scaling |
There was a problem hiding this comment.
Remove "I prefer" comment or change wording
| def log_fug_phase_comp_eq(b, p, j, pp): | ||
| if (p, j) not in b.phase_component_set: | ||
| raise KeyError(f"Component {j} is not present in phase {p}.") | ||
| # TODO does allowing a calculation with |
There was a problem hiding this comment.
Change wording on TODO message
| cobj = b.params.get_component(j) | ||
| if b.params.has_inherent_reactions: | ||
| raise PropertyNotSupportedError( | ||
| "Bubble/dew calculations for systems with inherent " |
There was a problem hiding this comment.
This is only a question. What would be required to support this?
There was a problem hiding this comment.
It depends. If all we want to do is have any temperature at which we can solve the VLE equations for SmoothVLE, then we can probably just use the current system composition in the bubble/dew calculations. However, if we want temperature_bubble and temperature_dew to be the system's actual bubble and dew temperatures, we would need to write inherent reaction equilibrium equations not just at the system's temperature, but also at the bubble/dew temperature. That adds a lot of complexity and overhead, so I'd rather punt on the issue until we have a specific problem we're trying to solve.
We don't have this issue with the CubicComplementarityVLE because the equilibrium temperature there is not directly related to the bubble/dew temperatures.
There was a problem hiding this comment.
Of course, we're not going to be using CubicComplementarityVLE with the Ideal EoS anyway.
agarciadiego
left a comment
There was a problem hiding this comment.
Just a couple of comments about commented code
Fixes
Idealequation of state now uses equilibrium temperature rather than actual temperatureRPP5now responds to configuration about whether or not to include enthalpy of formation in enthalpy calculationsInitializerBaseto decrease the scope of try/except block (hasattris implemented internally with atry/exceptblock)Enhancements
Idealnow uses log-form variables for mole fractionRPP5Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: