Skip to content

Conversation

@jsiirola
Copy link
Member

Fixes #1570.

Summary/Motivation:

@andrewlee94 reported strangeness with units where the following did not raise an error:

m = ConcreteModel()
m.v = Var(units=units.dimensionless)
m.v.set_value(42*units.s)

This was due to get_units() returning None instead of dimensionless. This caused Pyomo to not be able to distinguish between Var(), and Var(units=units.dimensionless). This PR changes the current behavior so that get_units() will always return a _PyomoUnit.

In addition, this PR reworks the PintUnitExtractionVisitor to make it slightly more performant.

While much of #1570 has already been fixed on main, this PR fixes the last known issues.

Changes proposed in this PR:

  • dimentionless is returned from get_units() instead of None
  • efficiency improvements in the PintUnitExtractionVisitor
  • cleaned up some long lines and string formatting

Legal Acknowledgement

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

  1. I agree my contributions are submitted under the BSD license.
  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.

@jsiirola jsiirola requested a review from carldlaird January 11, 2023 22:35
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Base: 87.13% // Head: 87.13% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (f2e3372) compared to base (7edd5f0).
Patch coverage: 90.24% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2691   +/-   ##
=======================================
  Coverage   87.13%   87.13%           
=======================================
  Files         757      757           
  Lines       84664    84674   +10     
=======================================
+ Hits        73770    73780   +10     
  Misses      10894    10894           
Flag Coverage Δ
linux 84.55% <90.24%> (+<0.01%) ⬆️
osx 74.58% <90.24%> (+<0.01%) ⬆️
other 84.73% <90.24%> (+<0.01%) ⬆️
win 81.89% <90.24%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyomo/core/base/units_container.py 97.87% <90.24%> (+0.05%) ⬆️
pyomo/solvers/plugins/solvers/ASL.py 91.42% <0.00%> (-1.43%) ⬇️
pyomo/core/expr/visitor.py 93.40% <0.00%> (+0.15%) ⬆️
pyomo/util/check_units.py 91.46% <0.00%> (+1.21%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

I don't know this code well enough to have a good idea of whether this is an efficient/good solution, but for what I do know, it looks fine to me (minus the smallest of typos).

@blnicho blnicho merged commit 85c3f33 into Pyomo:main Jan 30, 2023
@jsiirola jsiirola deleted the units-dimensionless branch January 30, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting var .value with units does not behave as expected

4 participants