-
Notifications
You must be signed in to change notification settings - Fork 62
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
Anaerobic Digester Re-Scaling #1535
base: main
Are you sure you want to change the base?
Conversation
watertap/property_models/unit_specific/anaerobic_digestion/tests/test_modified_adm1_reaction.py
Outdated
Show resolved
Hide resolved
iscale.set_scaling_factor(self.KH_co2, 1e2) | ||
iscale.set_scaling_factor(self.KH_ch4, 1e2) | ||
iscale.set_scaling_factor(self.KH_h2, 1e2) | ||
iscale.set_scaling_factor(self.hydraulic_retention_time, 1e-6) | ||
iscale.set_scaling_factor(self.volume_AD, 1e-2) | ||
iscale.set_scaling_factor(self.volume_vapor, 1e-2) | ||
iscale.set_scaling_factor(self.liquid_phase.rate_reaction_generation, 1e4) | ||
iscale.set_scaling_factor(self.liquid_phase.mass_transfer_term, 1e2) | ||
iscale.set_scaling_factor(self.liquid_phase.rate_reaction_extent, 1e4) | ||
iscale.set_scaling_factor(self.liquid_phase.enthalpy_transfer, 1e0) | ||
iscale.set_scaling_factor(self.liquid_phase.volume, 1e-2) | ||
iscale.set_scaling_factor(self.electricity_consumption, 1e0) |
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.
Shouldn't these be handled in the scaler class?
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.
Andrew mentioned that we should not mix and match the new and old scaling tools. In other words, the scaler class should not make use of iscale. This is the reason why I've moved all of the iscale.set_scaling_factor
to the calculate_scaling_factors
function (in this PR and in others), which won't be called when exclusively using the new scaling tools.
If your question is why don't I set these default scaling factors in the ADScaler class, it's because I figured these would make more sense as user-defined scaling factors. I know I did do some testing with having default scaling factors for these variables in the new scaler class, but I forget how thoroughly I played around with this. Currently, the only variable scaled by default in the new scaler class is volume.
@@ -56,71 +56,71 @@ def test_solve(self, system_frame): | |||
0.24219, rel=1e-3 | |||
) |
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.
Would it make sense to check the condition number at the flowsheet level between old scaling approach and new?
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.
Then, every time another PR impacts scaling at this flowsheet level, we can track whether condition number improves or not as we go.
Noting that the ScalingProfiler cannot currently be applied to the AD unit model since the ADM1 and modified ADM1 reaction packages have potential divisions by zero (see |
Interesting note. What happens exactly? Does ScalingProfiler do some sort of check for potential division by zero beforehand and raise an exception, or do you get back some exception that occurred having to do with division by zero? It’d be good to elaborate on this. |
This is the traceback:
|
Summary/Motivation:
Adds the new IDAES scaling routine (scaler objects) to the anaerobic digester unit model. This should be merged after #1519 and #1530 are merged in.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: