Skip to content

Fix get default scaling factor#1659

Merged
dallan-keylogic merged 18 commits intoIDAES:scaling_toolboxfrom
dallan-keylogic:fix_get_default_scaling_factor
Sep 26, 2025
Merged

Fix get default scaling factor#1659
dallan-keylogic merged 18 commits intoIDAES:scaling_toolboxfrom
dallan-keylogic:fix_get_default_scaling_factor

Conversation

@dallan-keylogic
Copy link
Contributor

@dallan-keylogic dallan-keylogic commented Sep 10, 2025 •
edited
Loading

Fixes

get_default_scaling_factor formerly looked for component.local_name in the default_scaling_factors dictionary. However, that caused problems for indexed variables, where the user could plausibly set default_scaling_factors[mole_frac_phase_comp[Liq, H2O]] and have it be ignored because of the space after the comma. This PR fixes that using the component finder. The implementation, which iterates over all entries in the dictionary, isn't particularly efficient, but we don't expect these dictionaries to be that long---we can always optimize later.

We also have the shortcoming that both the keys mole_frac_phase_comp[Liq, H2O] and mole_frac_phase_comp[Liq,H2O] (without a space after the comma) can coexist in the same dictionary. The key that is encountered first will have its scaling factor returned. There should probably be some validation step that screens for duplicates. That can be addressed when we address #1648 item 3.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@codecov
Copy link

codecov bot commented Sep 10, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.09%. Comparing base (29f1e5c) to head (77b8bac).
⚠️ Report is 28 commits behind head on scaling_toolbox.

Additional details and impacted files 
@@               Coverage Diff                @@
##           scaling_toolbox    #1659   +/-   ##
================================================
  Coverage            77.09%   77.09%           
================================================
  Files                  395      395           
  Lines                63171    63182   +11     
  Branches             10373    10380    +7     
================================================
+ Hits                 48702    48713   +11     
  Misses               12021    12021           
  Partials              2448     2448

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Sep 11, 2025
@ksbeattie ksbeattie requested a review from blnicho September 11, 2025 18:30
@dallan-keylogic
Copy link
Contributor Author

dallan-keylogic commented Sep 12, 2025

I think this might be running afoul of the create-on-demand properties---by using find_component on every entry in the default_scaling_factors dictionary, it tries to create those components. I think that explains some test failures in #1652 . I know there's the lock_attribute_creation_context() context manager that we could use--- @blnicho is that the most appropriate choice?

@blnicho
Copy link
Member

blnicho commented Sep 17, 2025

@dallan-keylogic the lock_attribute_creation_context is new to me but this seems like its intended purpose.

blnicho
blnicho reviewed Sep 24, 2025
View reviewed changes
Copy link
Member

@blnicho blnicho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dallan-keylogic I have a few minor change requests but otherwise this looks fine

@dallan-keylogic dallan-keylogic merged commit ca3c1fd into IDAES:scaling_toolbox Sep 26, 2025
39 checks passed
@dallan-keylogic dallan-keylogic deleted the fix_get_default_scaling_factor branch September 26, 2025 13:45
dallan-keylogic added a commit that referenced this pull request Nov 6, 2025
@dallan-keylogic @blnicho
Fix get default scaling factor (#1659)
c7e806a 
* error for unnamed expressions

* pylint

* default is warning=False

* make sure expression walker doesn't emit warnings

* fix get_default_scaling_factor

* no build on demand

* not every component has lock attribute creation context

* rescue files from different branch

* avoid duplicate code and add tests

* remove unused variable

* fix spelling

* pylint

* changes suggested from review

---------

Co-authored-by: Bethany Nicholson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blnicho blnicho blnicho approved these changes

@agarciadiego agarciadiego agarciadiego approved these changes

@lbianchi-lbl lbianchi-lbl Awaiting requested review from lbianchi-lbl

Assignees

@dallan-keylogic dallan-keylogic

Labels

Priority:Normal Normal Priority Issue or PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dallan-keylogic @blnicho @agarciadiego @ksbeattie