Skip to content
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

Update imports of native_types and pyomo_constant_types in scaling module #1366

Merged
merged 4 commits into from
Mar 13, 2024

Conversation

Robbybp
Copy link
Member

@Robbybp Robbybp commented Mar 6, 2024

These should be imported from pyomo.common.numeric_types. Also, pyomo_constant_types was deprecated last week (although I don't do anything about that). This PR is necessary to prevent import errors with pyomo/main, as pyomo_constant_types no longer exists in pyomo.core.expr.numvalue.

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.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.59%. Comparing base (fc1a7da) to head (1951915).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1366      +/-   ##
==========================================
- Coverage   77.60%   77.59%   -0.01%     
==========================================
  Files         391      391              
  Lines       64330    64330              
  Branches    14244    14244              
==========================================
- Hits        49923    49920       -3     
- Misses      11831    11835       +4     
+ Partials     2576     2575       -1     

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

Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Mar 7, 2024
@ksbeattie ksbeattie requested a review from jsiirola March 7, 2024 19:31
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.

Looks good to me. If pyomo_constant_types is deprecated, we probably should open an issue to move away from using it eventually.

@andrewlee94 andrewlee94 enabled auto-merge (squash) March 11, 2024 15:31
@jsiirola
Copy link
Contributor

jsiirola commented Mar 11, 2024

I would also recommend changing the following line (and removing the import of pyomo_constant_types):

diff --git a/idaes/core/util/scaling.py b/idaes/core/util/scaling.py
index d4bd825..f3ee1cf 100644
--- a/idaes/core/util/scaling.py
+++ b/idaes/core/util/scaling.py
@@ -1509,7 +1509,7 @@ class NominalValueExtractionVisitor(EXPR.StreamBasedExpressionVisitor):
         # first check if the node is a leaf
         nodetype = type(node)

-        if nodetype in native_types or nodetype in pyomo_constant_types:
+        if nodetype in native_types:
             return [node]

         node_func = self.node_type_method_map.get(nodetype, None)

@andrewlee94
Copy link
Member

@Robbybp Would you be able to add the addition change @jsiirola suggested?

@lbianchi-lbl lbianchi-lbl enabled auto-merge (squash) March 13, 2024 04:01
@lbianchi-lbl lbianchi-lbl merged commit 38e67db into IDAES:main Mar 13, 2024
45 checks passed
@Robbybp Robbybp deleted the consttype-import branch March 13, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants