-
Notifications
You must be signed in to change notification settings - Fork 517
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
Consolidate walker logic in LP/NL representations #3015
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3015 +/- ##
==========================================
+ Coverage 87.84% 87.87% +0.03%
==========================================
Files 769 769
Lines 89541 89516 -25
==========================================
+ Hits 78657 78665 +8
+ Misses 10884 10851 -33
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool--I like it! Mostly just questions to make sure I'm understanding...
""" | ||
A utility function for updating the set of types that are | ||
recognized to handle numeric values. | ||
def RegisterNumericType(new_type: type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really bother me (as that would by hypocritical), but wouldn't it be PEP8-ier for these be snake case rather than camel case since they're functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - they should be snake case. But as this has been an "official" API for a long time, we would need to rename it with a deprecation path (which I'm OK with, but maybe as a separate PR?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. A separate PR makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do that... We have these in several places.
expressions. The type should be compatible with :py:class:`float` | ||
(that is, store a scalar and be castable to a Python float). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a copy-paste typo? Should they be castable to int
rather than float
? (I mean, Python is happy to cast floats to ints but...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Believe it or not, it is not a typo. The assumption in the Numeric expression system is that the value of any expression should be float-like.
This set is actually not used anywhere in Pyomo. I think it was added in the dawn of time on speculation that it might be useful. Maybe the right thing is to deprecate it entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be in favor of deprecating it before someone starts using it and we get really confused. :P
def RegisterLogicalType(new_type: type): | ||
"""Register the specified type as a "logical type". | ||
|
||
A utility function for registering new types as "native logical | ||
types". Logical types can be leaf nodes in Pyomo logical | ||
expressions. The type should be compatible with :py:class:`bool` | ||
(that is, store a scalar and be castable to a Python bool). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What goes in native_logical_types
that doesn't go in native_boolean_types
? Or is this a backwards compatibility thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a backwards compatibility thing. native_boolean_types
is the deprecated set and was replaced by native_logical_types
. I toyed with making them explicit aliases for each other, but elected to not continue mucking with a deprecated API.
_CONSTANT = ExprType.CONSTANT | ||
|
||
|
||
class BeforeChildDispatcher(collections.defaultdict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, this is very cute... I like it.
# @staticmethod | ||
# def _before_var(visitor, child): | ||
# pass | ||
|
||
# @staticmethod | ||
# def _before_named_expression(visitor, child): | ||
# pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly because I don't like ABCMeta for classes in tight loops. These are two methods that are required by register_dispatcher
, but are not implemented in the base class (the LP and NL implementations are completely different). They are commented out to remind developers of an expected API...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't the advantage of ABCMeta that it will actually yell at you when you don't implement the things you expect to be there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Emma's comment.
if offset.__class__ not in int_float: | ||
offset = float(offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you have to cast these anymore? Is it a change from this PR, or was it already redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new walkers are better at catching non-float-like types when processing the leaf nodes, so we no longer have to re-verify the return types from the walker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One NFC that you can ignore if you want (it's a typo that crate-ci doesn't catch, but I added it to the October list, so it'll be caught soon regardless).
""" | ||
A utility function for updating the set of types that are | ||
recognized to handle numeric values. | ||
def RegisterNumericType(new_type: type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do that... We have these in several places.
# @staticmethod | ||
# def _before_var(visitor, child): | ||
# pass | ||
|
||
# @staticmethod | ||
# def _before_named_expression(visitor, child): | ||
# pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Emma's comment.
Fixes # .
Summary/Motivation:
This PR reduces repetition in the codebase by consolidating
beforeChild
andexitNode
logic in the LP and NL representation walkers into standard implementations ofBeforeChildDispatcher
andExitNodeDispatcher
. By consolidating logic we can ensure that all coefficients / values in the walker return data structures are guaranteed to benative_numeric_types
, allowing us to simplify (and slightly speed up) the LP and NL walkers.As part of this, we are making a change in the definition of the
native_numeric_types
set by removingcomplex
from that set, and registering complex types in a separatenative_complex_types
set. This change makes for a more consistent definition ofnative_numeric_types
: it contains only data types that are "equivalent to float" and can be emitted as part of standard solver interfaces.Changes proposed in this PR:
BeforeChildDispatcher
andExitNodeDispatcher
classes to centralize the definition and management of the multiple dispatch dictionaries used in the representation walkerscomplex
fromnative_numeric_types
(and put in a separatenative_complex_types
setLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: