-
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
Redefine objective sense as a proper IntEnum
#3224
Conversation
…y match what is advertised in the Book
IntEnum
IntEnum
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 like this! I have one question about the SCIP change below--just wondering if you could fall victim to numerical issues.
if results.problem.sense == ProblemSense.minimize: | ||
if results.solver.primal_bound < results.solver.dual_bound: | ||
results.problem.lower_bound = results.solver.primal_bound | ||
results.problem.upper_bound = results.solver.dual_bound | ||
else: | ||
results.problem.lower_bound = results.solver.dual_bound | ||
results.problem.upper_bound = results.solver.primal_bound |
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.
Can this go wrong if your bounds cross by a little bit?
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 - you probably can. This will be easily fixable in the new solver interfaces (where the results parser can have easier access to the original model and won't have to guess about the sense), but there isn't an obvious better solution here (unless the scip output log prints something about the sense - but I really don't want to put more dependence on the parsing of output logs).
Fixes #993 .
Summary/Motivation:
There have been some longstanding inconsistencies regarding how we manage the definition of "objective sense" in Pyomo. The results object uses an Enum with 3 values (minimize, maximize, and unknown). In the modeling side, we make heavy use of two named constants (
minimize
andmaximize
). These constants are justint
s (1 and -1, respectively), and those constants are defined inpyomo.core.kernel.objective
(causing one of the few inter-linkages between kernel and the AML).This PR defines a new Enum (
pyomo.common.enums.ObjectiveSense
), and redefines theminimize
andmaximize
constants to be the members of that Enum. To preserve consistency / backwards compatibility,ObjectiveSense
is anIntEnum
, so the old constants / old usage does not need to change.ProblemSense
is now an extension ofObjectiveSense
that also admitsunknown
. Unfortunately, due to the way that the old solvers are structured, it is not (easy) to remove the use ofunknown
when processing results from the old solver interfaces. The compromise here is the development ofExtendedEnumType
, which allows us to define extended (or derived) Enums that combine the enums from another class with additional locally-defined members - so theminimize
andmaximize
members inProblemSense
are actually members ofObjectiveSense
.Changes proposed in this PR:
ExtendedEnumType
metaclass to support extended/derived EnumsObjectiveSense
enum (withminimize
andmaximize
members)ProblemSense
to be an extension ofObjectiveSense
ProblemSense
from almost all of PyomoLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: