Modular Properties Scaler Object#1652
Merged
dallan-keylogic merged 113 commits intoIDAES:scaling_toolboxfrom Sep 30, 2025
Merged
Modular Properties Scaler Object#1652dallan-keylogic merged 113 commits intoIDAES:scaling_toolboxfrom
dallan-keylogic merged 113 commits intoIDAES:scaling_toolboxfrom
Conversation
dallan-keylogic
commented
Sep 18, 2025
idaes/models/properties/modular_properties/base/tests/test_generic_property.py
Outdated
Show resolved
Hide resolved
dallan-keylogic
commented
Sep 18, 2025
dallan-keylogic
commented
Sep 18, 2025
dallan-keylogic
commented
Sep 18, 2025
Contributor
Ryan-Hughes-8
left a comment
Ryan-Hughes-8
left a comment
There was a problem hiding this comment.
Overall, I think this looks good. I tested this branch locally using an air/flue gas generic parameter block and the ethylene glycol production generic properties and reactions in the examples, and it worked well.
My main recommendation is improving the error message for when a required default scaling factor is omitted. I also left a couple other comments and suggestions.
| log_con_obj[idx], sf, overwrite=overwrite | ||
| ) | ||
|
|
||
| def _bubble_dew_scaling( |
Contributor
There was a problem hiding this comment.
I think this function, along with some others throughout this PR, could use some more comments to help explain what is being done.
idaes/models/properties/modular_properties/examples/tests/test_BTIdeal.py
Outdated
Show resolved
Hide resolved
idaes/models/properties/modular_properties/examples/tests/test_BTIdeal_FPhx.py
Outdated
Show resolved
Hide resolved
idaes/models/properties/modular_properties/examples/tests/test_BTIdeal_FcPh.py
Outdated
Show resolved
Hide resolved
idaes/models/properties/modular_properties/examples/tests/test_BTIdeal_FcTP.py
Outdated
Show resolved
Hide resolved
idaes/models/properties/modular_properties/examples/tests/test_CO2_bmimPF6_PR.py
Outdated
Show resolved
Hide resolved
| ) | ||
| # Phase equilibrium | ||
| # Right now (7/24/25) phase equilibrium methods don't create | ||
| # additional variables, so this method does nothing. |
Contributor
There was a problem hiding this comment.
remove "this method does nothing"
Contributor
There was a problem hiding this comment.
This is not the wording a user should read. Leave the comment just change the wording
| for vardata in log_var_obj.values(): | ||
| self.set_component_scaling_factor(vardata, 1, overwrite=overwrite) | ||
|
|
||
| # Not porting these from the old scaler, we'll see if |
| assert get_scaling_factor(model.rblock[1].dh_rxn["r1"]) == pytest.approx( | ||
| 1 / (300 * 8.3144), rel=1e-3 | ||
| ) | ||
| assert get_scaling_factor(model.rblock[1].k_eq["e1"]) == 0.01 |
Contributor
There was a problem hiding this comment.
Question: Why are we changing some tests to have "1e^" form and some to decimals?
Contributor
Author
There was a problem hiding this comment.
I think I copy and pasted the test, then began changing the wrong version, only to imperfectly revert it. I restored the changes in this file
| Scaler for constraints associated with FcTP state variables | ||
| """ | ||
|
|
||
| # Safe to inherit variable_scaling_routine from FTPx |
Contributor
There was a problem hiding this comment.
Standarize this comments through state definitions. FpcTP does not include "safe".
Ryan-Hughes-8
approved these changes
Sep 30, 2025
agarciadiego
approved these changes
Sep 30, 2025
dallan-keylogic
added a commit
that referenced
this pull request
Nov 6, 2025
* rescue files from overloaded git branch * fix due to api tweaks * run black * forgot to add a file * fix test errors * pin coolprop version * update version, disable superancillaries * run black * respond to Marcus's feedback * getting close * address Will's comments * tests for set_scaling_factor * support for unions in python 3.9 * testing the scaling profiler is way too fragile * modify test to be less fragile * remove pdb * rescue files from branch * towards scaling cv * preliminary testing * scaling by defn constraint * scale constraint by definition constraint * Disable obsolete tests for now * run black * actually add tests * inh * tests for methods rescued from old scaling tools * pylint * test to make sure that value is reverted * Apply suggestions from code review spelling fixes Co-authored-by: Brandon Paul <[email protected]> * additional clarity * more files rescued from branch * rescue files from other branch * first tests * tests * additional scaling * more tests * fixes * run black * black on forgotten file * fix failing tests * fix issues when initializing FpcTP * begin scaling for cubic complementarity vle * error for unnamed expressions * pylint * enthalpy of formation test * default is warning=False * make sure expression walker doesn't emit warnings * Cubic complementarity VLE test * avoid auto-construction and run black * new scaling for test_BTIdeal_FcTP * BT ideal test * regularly scheduled scaling has been interrupted by a bug in scaling core * fix get_default_scaling_factor * tests for one more example ported * delegate scaling for solubility product forms * test FcPh * test FcTP * test_FpcTP * test FTPx * FPhx * no build on demand * not every component has lock attribute creation context * no more enth_mol_phase * run black * move to scaler object get scaling factor for future extension * get rid of remaing gsf and pylint changes * Actually scale cubic complementarity VLE and pylint * rescue files from different branch * tests for scaler base get_scaling_factor * remove dependency on fix_gdsf * pylint * maybe pylint will like this better * Is this acceptable to both pylint and black? * stash * fix failing test * black * pylint * minor tweaks * increase test coverage * avoid generation of properties * more test coverage for IdealBubbleDew * Renaming and comments * fix wrong constraint scaling method * reply to review comments * revert legacy test * failing test * improve error message * update authors * forgot to save file * fix exception match * Let's see if three solves is enough * less strict complementarity * xfail --------- Co-authored-by: Brandon Paul <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary/Motivation:
I've begun implementing scaling for the modular property framework.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: