Skip to content

Conversation

@jsiirola
Copy link
Member

@jsiirola jsiirola commented Dec 4, 2021

Fixes # .

Summary/Motivation:

This PR adds a as_quantity() function that will accept numeric-like objects (int / float / None / Var / Param / expression / etc) and return the value as a native pint Quantity (value + units).

This is useful for applications like user interfaces, and was requested by IDAES (@andrewlee94 and @dangunter)

Changes proposed in this PR:

  • add as_quantity() function to evaluate an expression to a pint Quantity
  • add tests (including expanded coverage of units_container)

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 December 4, 2021 04:04
@codecov
Copy link

codecov bot commented Dec 4, 2021

Codecov Report

Merging #2222 (9016240) into main (0d35f56) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2222      +/-   ##
==========================================
+ Coverage   82.74%   82.77%   +0.02%     
==========================================
  Files         607      607              
  Lines       76792    76843      +51     
==========================================
+ Hits        63544    63608      +64     
+ Misses      13248    13235      -13     
Impacted Files Coverage Δ
pyomo/core/base/units_container.py 98.03% <100.00%> (+2.28%) ⬆️
pyomo/environ/__init__.py 82.35% <100.00%> (ø)
pyomo/solvers/plugins/solvers/ASL.py 91.42% <0.00%> (+1.42%) ⬆️
pyomo/core/base/boolean_var.py 79.61% <0.00%> (+1.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d35f56...9016240. Read the comment docs.

}

def as_quantity(expr):
return _QuantityVisitor().dfs_postorder_stack(expr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a comment, not a required change - should we standardize this at some point to change all instances that use dfs_postorder_stack to walk_expression?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eventually? Yes. But that will require reworking the ExpressionValueVisitor to be based on the StreamBasedExpressionVisitor.

Copy link
Member

@carldlaird carldlaird left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me.

return "(" + _str + ")"
else:
return str(self)
return _str
Copy link
Member

Choose a reason for hiding this comment

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

Is this mostly for debugging, or is it used somewhere else?

if hasattr(node, 'get_units'):
unit = node.get_units()
if unit is not None:
return True, value(node) * unit._pint_unit
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this works. If a Var has units that are themselves an expression (e.g., kg*m), then that expression won't have "_pint_unit"?

Copy link
Member

Choose a reason for hiding this comment

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

After discussions with @jsiirola I guess we create compound units instead of keeping the expression. Let's make sure there are tests that include Vars with units that are expressions.

Copy link
Member

@carldlaird carldlaird left a comment

Choose a reason for hiding this comment

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

This causes a broader question for the team. This PR is officially exposing pint from behind the Pyomo units. Do we want to do this?

I have no real concerns, however, we did quite a bit of work to make sure that pint was abstracted away (so we could replace pint with something else later if we wanted to). This decision may have been more about being unfamiliar with pint and not "ready to commit" to it. If we are ready to commit to pint, then there may be other simplifications we can make.

Just thoughts for discussion.

@blnicho
Copy link
Member

blnicho commented Jan 5, 2022

@carldlaird we discussed your question at the developers meeting this week and the consensus was that we didn't have any concerns with exposing more of pint. Given the currently available alternatives to pint we think the likelihood of replacing it with something else is pretty slim.

@blnicho blnicho merged commit 178d701 into Pyomo:main Jan 11, 2022
@jsiirola jsiirola deleted the as_quantity branch February 22, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants