Skip to content

Comments

Fix set_parameter_path in Helmholtz Property Package#1530

Merged
ksbeattie merged 8 commits intoIDAES:mainfrom
bertkdowns:main
Feb 13, 2025
Merged

Fix set_parameter_path in Helmholtz Property Package#1530
ksbeattie merged 8 commits intoIDAES:mainfrom
bertkdowns:main

Conversation

@bertkdowns
Copy link
Contributor

@bertkdowns bertkdowns commented Nov 11, 2024

Fixes

Fix the set_parameter_path() function in helmholtz.
update helmholtz_functions.py and helmholtz_state.py to use the latest cfg variable rather than storing a copy

Summary/Motivation:

the set_parameter_path function does not properly update the helmholtz parameter file path, because it sets idaes.properties.helmholtz.parameter_file_path rather than idaes.cfg.properties.parameter_file_path (note .cfg).

Also, helmholtz_functions.py makes a copy of the cfg variable into _data_dir, which also is not updated with set_parameter_path.

This involves needing to do some hacky workarounds to specify a different directory to load helmholtz data from (e.g see my register_compounds function.

Changes proposed in this PR:

  • The _data_dir variable is replaced with a function that gets the config variable
  • The set_parameter_path function is fixed to update the config variable correctly
  • the compounds are auto-registered, as specified in the set_parameter_path documentation

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.

@dallan-keylogic
Copy link
Contributor

dallan-keylogic commented Nov 13, 2024

@bertkdowns Could you please format your PR with Black? I'd like to see if it breaks the CI.

@bertkdowns
Copy link
Contributor Author

bertkdowns commented Nov 13, 2024 •
edited
Loading

@bertkdowns Could you please format your PR with Black? I'd like to see if it breaks the CI.

done, and the pytest tests passed on my machine, except for the ones that required the petsc solver as I didn't have it installed

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Nov 14, 2024
@dallan-keylogic
Copy link
Contributor

dallan-keylogic commented Nov 15, 2024

Looks like tests are passing, random Keras/multithreading errors nonwithstanding. @eslickj , if you get a chance, could you take a look to see if this breaks anything? At any rate, we should probably wait until after the 2.7 release to merge this.

@ksbeattie
Copy link
Member

ksbeattie commented Dec 19, 2024

@lbianchi-lbl , I think you were going to take a look at this (when you get back)?

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.05%. Comparing base (6961bb5) to head (8262ab3).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...eneral_helmholtz/components/parameters/__init__.py 0.00% 2 Missing ⚠️
...roperties/general_helmholtz/helmholtz_functions.py 95.91% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
- Coverage   77.06%   77.05%   -0.01%     
==========================================
  Files         389      389              
  Lines       62680    62683       +3     
  Branches    10276    10276              
==========================================
- Hits        48303    48302       -1     
- Misses      11939    11944       +5     
+ Partials     2438     2437       -1

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

@eslickj
Copy link
Contributor

eslickj commented Dec 20, 2024

As far as I can tell it looks good.

@ksbeattie ksbeattie added the CI:run-integration triggers_workflow: Integration label Jan 16, 2025
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Jan 16, 2025
dallan-keylogic
dallan-keylogic approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@dallan-keylogic dallan-keylogic left a comment

Choose a reason for hiding this comment

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

LGTM

@lbianchi-lbl lbianchi-lbl added the CI:run-integration triggers_workflow: Integration label Jan 30, 2025
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Jan 30, 2025
@ksbeattie ksbeattie added the CI:run-integration triggers_workflow: Integration label Feb 6, 2025
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Feb 6, 2025
@ksbeattie ksbeattie merged commit 88ca7c1 into IDAES:main Feb 13, 2025
64 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lbianchi-lbl lbianchi-lbl lbianchi-lbl approved these changes

@dallan-keylogic dallan-keylogic dallan-keylogic approved these changes

@agarciadiego agarciadiego Awaiting requested review from agarciadiego agarciadiego is a code owner

Assignees

@bertkdowns bertkdowns

@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.

7 participants

@bertkdowns @dallan-keylogic @ksbeattie @codecov-commenter @eslickj @lbianchi-lbl @idaes-build