-
Notifications
You must be signed in to change notification settings - Fork 294
Addition of the FpTPxpc state definition with required testing #1710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…nding the property
…se test. Uses a FeedFlash unit model rather than a state block to perform the test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1710 +/- ##
==========================================
+ Coverage 77.45% 77.48% +0.02%
==========================================
Files 395 397 +2
Lines 64792 64992 +200
Branches 10902 10931 +29
==========================================
+ Hits 50186 50357 +171
- Misses 12095 12117 +22
- Partials 2511 2518 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dallan-keylogic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this looks good. There are a few minor changes that are necessary. However, in addition to the old-style calculate_scaling_factors method, you should include a new style scaler object. The other state definitions now have scaler objects that you can use as an example.
idaes/models/properties/modular_properties/examples/tests/test_BTIdeal_FpTPxpc.py
Outdated
Show resolved
Hide resolved
idaes/models/properties/modular_properties/examples/tests/test_BTIdeal_FpTPxpc.py
Outdated
Show resolved
Hide resolved
idaes/models/properties/modular_properties/examples/BT_ideal_FpTPxpc.py
Outdated
Show resolved
Hide resolved
idaes/models/properties/modular_properties/state_definitions/tests/test_FpTPxpc.py
Outdated
Show resolved
Hide resolved
…ant config properties dictionary and file
tannerpolley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved comments made by Doug
…nstraint' based on `has_phase_equilibrium` boolean. Also additional conditional in blocking the activiation of the `equilibrium_constraint` if `mole_frac_phase_comp` is a state variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like commented-out code is causing test failures.
| # if self.params.config.phases_in_equilibrium is not None: | ||
| # if self.always_flash: | ||
| # has_phase_equilibrium = True | ||
| # elif self.config.defined_state: | ||
| # has_phase_equilibrium = False | ||
| # else: | ||
| # has_phase_equilibrium = self.config.has_phase_equilibrium | ||
| # | ||
| # if has_phase_equilibrium: | ||
| # t_units = self.params.get_metadata().default_units.TEMPERATURE | ||
| # if self.temperature.value is not None: | ||
| # t_value = value(self.temperature) | ||
| # else: | ||
| # t_value = None | ||
| # self._teq = Var( | ||
| # self.params._pe_pairs, | ||
| # initialize=t_value, | ||
| # doc="Temperature for calculating phase equilibrium", | ||
| # units=t_units, | ||
| # bounds=self.temperature.bounds, | ||
| # ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code commented out?
| # if self.params.config.phases_in_equilibrium is not None: | ||
| # if self.always_flash: | ||
| # has_phase_equilibrium = True | ||
| # elif self.config.defined_state: | ||
| # has_phase_equilibrium = False | ||
| # else: | ||
| # has_phase_equilibrium = self.config.has_phase_equilibrium | ||
| # | ||
| # if has_phase_equilibrium: | ||
| # | ||
| # pe_form_config = self.params.config.phase_equilibrium_state | ||
| # for pp in self.params._pe_pairs: | ||
| # pe_form_config[pp].phase_equil(self, pp) | ||
| # | ||
| # def rule_equilibrium(b, phase1, phase2, j): | ||
| # if (phase1, j) not in b.phase_component_set or ( | ||
| # phase2, | ||
| # j, | ||
| # ) not in b.phase_component_set: | ||
| # return Constraint.Skip | ||
| # config = b.params.get_component(j).config | ||
| # try: | ||
| # e_mthd = config.phase_equilibrium_form[ | ||
| # (phase1, phase2) | ||
| # ].return_expression | ||
| # except KeyError: | ||
| # e_mthd = config.phase_equilibrium_form[ | ||
| # (phase2, phase1) | ||
| # ].return_expression | ||
| # if e_mthd is None: | ||
| # raise GenericPropertyPackageError(b, "phase_equilibrium_form") | ||
| # return e_mthd(self, phase1, phase2, j) | ||
| # | ||
| # self.equilibrium_constraint = Constraint( | ||
| # self.params._pe_pairs, self.component_list, rule=rule_equilibrium | ||
| # ) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code commented out?
| @pytest.mark.component | ||
| def test_solve(self, model): | ||
| results = solver.solve(model) | ||
|
|
||
| # Check for optimal solution | ||
| assert check_optimal_termination(results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you solving this block here? You shouldn't be able to solve it on its own, only when it's paired with a ControlVolume that can create phase equilibrium terms in the material balance.
|
|
||
| b.eq_mole_frac_tbub = Constraint( | ||
| b.params._pe_pairs, b.component_list, rule=rule_mole_frac_bubble_temp | ||
| b.params._pe_pairs_vle_comp, rule=rule_mole_frac_bubble_temp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be persuaded that this change is necessary, but I would rather not make it without a strong justification.
|
|
||
| b.eq_mole_frac_pbub = Constraint( | ||
| b.params._pe_pairs, b.component_list, rule=rule_mole_frac_bubble_press | ||
| b.params._pe_pairs_vle_comp, rule=rule_mole_frac_bubble_press |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be persuaded that this change is necessary, but I would rather not make it without a strong justification.
|
|
||
| b.eq_pressure_dew = Constraint( | ||
| b.params._pe_pairs, b.component_list, rule=rule_dew_press | ||
| b.params._pe_pairs_vle_comp, rule=rule_dew_press |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be persuaded that this change is necessary, but I would rather not make it without a strong justification.
| # | ||
| # m = model.fs.state[0] | ||
| # | ||
| # m.display() | ||
| # # | ||
| # # for v in m.component_data_objects(Var, active=True): | ||
| # # print(f"{v.name}: value={v.value}, lb={v.lb}, ub={v.ub}") | ||
| # # | ||
| # # print() | ||
| # # | ||
| # # for c in m.component_data_objects(Constraint, active=True): | ||
| # # print(c.name, c.expr) | ||
| # | ||
| # # As the phase equilibrium constraints were not solved, we expect these to have a large residual | ||
| # large_res = large_residuals_set(model.fs.state[0]) | ||
| # print(large_res) | ||
| # assert len(large_res) == 4 | ||
| # for i in large_res: | ||
| # assert i.name in [ | ||
| # "fs.state[0.0].phase_fraction_constraint[Liq]", | ||
| # "fs.state[0.0].phase_fraction_constraint[Vap]", | ||
| # "fs.state[0.0].equilibrium_constraint[Vap,Liq,H2O]", | ||
| # "fs.state[0.0].equilibrium_constraint[Vap,Liq,CO2]", | ||
| # ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out?
| m.fs.state[1].mole_frac_phase_comp["Liq", "H2O"].fix( | ||
| 1 / 3 / value(m.fs.state[1].flow_mol_phase["Liq"]) | ||
| ) | ||
| m.fs.state[1].mole_frac_phase_comp["Liq", "CO2"].fix( | ||
| 1 / 3 / value(m.fs.state[1].flow_mol_phase["Liq"]) | ||
| ) | ||
| m.fs.state[1].mole_frac_phase_comp["Liq", "KHCO3"].fix( | ||
| 1 / 3 / value(m.fs.state[1].flow_mol_phase["Liq"]) | ||
| ) | ||
| m.fs.state[1].mole_frac_phase_comp["Vap", "H2O"].fix( | ||
| 1 / 6 / value(m.fs.state[1].flow_mol_phase["Vap"]) | ||
| ) | ||
| m.fs.state[1].mole_frac_phase_comp["Vap", "CO2"].fix( | ||
| 1 / 6 / value(m.fs.state[1].flow_mol_phase["Vap"]) | ||
| ) | ||
| m.fs.state[1].mole_frac_phase_comp["Vap", "KHCO3"].fix( | ||
| 1 / 6 / value(m.fs.state[1].flow_mol_phase["Vap"]) | ||
| ) | ||
| m.fs.state[1].mole_frac_phase_comp["Vap", "N2"].fix( | ||
| 0.5 / value(m.fs.state[1].flow_mol_phase["Vap"]) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you dividing by flow_mol_phase here? This seems like a leftover from the FpcTP test.
| print() | ||
| print(j) | ||
| print(value(m.fs.state[1].flow_mol_phase_comp_apparent["Liq", j])) | ||
| print(value(m.fs.state[1].flow_mol), value(m.fs.state[1].mole_frac_comp[j])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't have random print statements in tests.
| ) | ||
|
|
||
| # Check apparent species mole fractions | ||
| m.fs.state[1].mole_frac_phase_comp_apparent.display() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
…changes to a future potential feature
8dbdb18 to
a563a22
Compare
|
This is being replaced by #1723 |
Fixes
Adds the state definition FpTPxpc with required testing
Summary/Motivation:
There were cases where the FpcTP state definition was failing with Modular Properties
Changes proposed in this PR:
general_propertyandsmooth_VLELegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: