Fix some bugs in the modular properties implementation #1425
Conversation
andrewlee94
added
bug
| @@ -700,7 +700,7 @@ def build(self): | |||
| j, | |||
| ) in self._phase_component_set: | |||
| # Component j is in both phases, in equilibrium | |||
| pe_dict["PE" + str(counter)] = {j: (pp[0], pp[1])} | |||
| pe_dict["PE" + str(counter)] = [j, (pp[0], pp[1])] | |||
There was a problem hiding this comment.
If we don't expect these objects to be mutated, wouldn't it be better to have them as tuples?
There was a problem hiding this comment.
This is from deeper in the framework so now would not be the time to change this. That said, a tuple might make sense here.
| # Default is no initialization of VLE | ||
| vap_frac = None |
There was a problem hiding this comment.
Is this change because of PyLint?
There was a problem hiding this comment.
No - this was from issue #1424. Later steps expect vap_frac to exist, so this is making sure it will exist no matter what course gets taken in the if/else block.
There was a problem hiding this comment.
Ah, I see. I looked at the different branches of the inner if statement, but forgot it was nested inside an outer if statement.
|
LGTM, I manually retriggered the coverage upload tests which should remove the block. |
* Fixing bug in modular phase equilibrium list * Add regression tests * Fix unrelated typo (cherry picked from commit 2ac6417)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1425 +/- ##
==========================================
- Coverage 77.79% 77.79% -0.01%
==========================================
Files 393 393
Lines 64797 64797
Branches 14390 14390
==========================================
- Hits 50412 50410 -2
- Misses 11795 11798 +3
+ Partials 2590 2589 -1 ☔ View full report in Codecov by Sentry. |
Fixes #1423, #1424
Summary/Motivation:
Two issues were recently raised identifying bugs in the modular properties code.
Changes proposed in this PR:
phase_equilibrium_listis created ingeneric_properties.pyso that is a list (as expected) and not a dict.Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: