-
Notifications
You must be signed in to change notification settings - Fork 228
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
Repair erroneous units for material_holdup in CV0D when mass basis defined #1460
Conversation
I think it might be that flowsheets are just allowed to have no units at all. In that case, you'd set |
@dallan-keylogic it looks like tests will fail for cases where dynamic=False and has_holdup=True, so I am going to setup holdup_units separately, under the conditional where has_holdup = True and right before material_holdup is defined. |
… needed for mass flow, preserving all previous use cases
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1460 +/- ##
=======================================
Coverage 76.38% 76.38%
=======================================
Files 394 394
Lines 65108 65121 +13
Branches 14423 14427 +4
=======================================
+ Hits 49732 49742 +10
- Misses 12814 12817 +3
Partials 2562 2562 ☔ View full report in Codecov by Sentry. |
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 think this looks good,
idaes/core/base/control_volume0d.py
Outdated
== MaterialFlowBasis.mass | ||
): | ||
holdup_units = units("mass") | ||
# TODO: Revisit the following: Preserving the original implementation and assigning units("amount") for molar flow and any other |
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.
What is the intention of this 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.
@andrewlee94 Since I am strictly attempting to fix the issue of using the correct units for holdup when dealing with mass as the material flow basis, here I am setting units to that of mass in that case, and for all other cases, use "amount" [mole], which was how the units were originally hard-coded for material holdup. So, I put this TODO note in case there were other conditionals that we wanted to account for (e.g., unitless, other cases).
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.
Ah - there is a third case which is other
, in which case we should assign units of None
(note not dimensionless
- it is a minor distinction but None
here indicates "we don't know" as opposed to actually being dimensionless).
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 suppose I can cover this with an else
to include other
and any other case that might arise, which seems to be consistent with how holdup_units is handled in the ControlVolume1D.
@dallan-keylogic The flowsheet is allowed to have no units, which is mostly a holdover from before units existed. However, in order for there to be a dynamic term, the |
@adam-a-a Whilst you are at it, could you check the 1D control volume as well to make sure it does not have the same issue. |
@andrewlee94 sure, I can do that. @dallan-keylogic let me know if you think there are edge cases that I should still account for in this PR or not. |
@andrewlee94 One problem with setting units to None in the |
So I checked this (assuming Next step would be to allow for unitless volume in such a case...but before I continue down what seems to be a relatively fruitless path, do you think I should do this, or should we consider being more restrictive and doing away with the unitless option? |
@adam-a-a That unit consistency is quite possible OK - with a flow basis of |
idaes/core/base/control_volume0d.py
Outdated
else: | ||
holdup_units = None | ||
# volume units will need to be updated to None for unit consistency in material_holdup_calculation | ||
self.volume._units = None |
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.
self.volume._units = None | |
self.volume._units = None |
@andrewlee94 - I am guessing you'd opt to remove this bit.
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.
Yes - I'd leave that as is and accept that units will not be consistent. We know what units volume will have from the property package, so we can assign them here.
For example, a flow-on effect of this would be that units would be inconsistent for rate based reactions (where we expect extent = volume*rate
). I.e. this is just pushing the error around to a different location. Also note that with a flow basis of other
we cannot convert a united reaction rate to the correct units either, so there is a potential second unit inconsistency that arises with flow basis of other
(hence my comment that when a user selects other
they are taking responsibility for unit consistency themselves).
@adam-a-a Could you confirm that you checked the 1D control volume and that the issue was not present there? |
@andrewlee94 - yes, I confirmed by reviewing the 1D CV code. I didn't go as far to test directly. The issue was not present there. |
@adam-a-a Thank you - adding a test would be a good idea if you could though (to make sure that it does not break in the future). |
@andrewlee94 OK, added a test to check 1d unit consistency when dynamic and mass basis used |
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.
LGTM
Fixes: #1461
Summary/Motivation:
This PR adjusts the units defined for the material_holdup variable constructed in ControlVolume0D, which currently only sets units as
mol
regardless of whether a mole or mass flow basis is defined. This is a problem when working with a mass flow basis.Notably, you would only notice this issue if you were constructing a dynamic ControlVolume0D using a mass flow basis, as we are starting to do on WaterTAP.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: