-
Notifications
You must be signed in to change notification settings - Fork 528
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
Extend valid types for PyROS solver argument uncertain_params
#3439
Conversation
…o into pyros-vars-as-params
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3439 +/- ##
==========================================
+ Coverage 88.19% 88.47% +0.28%
==========================================
Files 880 880
Lines 100594 100639 +45
==========================================
+ Hits 88715 89044 +329
+ Misses 11879 11595 -284
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…o into pyros-vars-as-params
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 question that should probably be addressed before merging. The other two are enhancements, but not strictly necessary.
if isinstance(obj, VarData): | ||
obj.fix() | ||
uncertain_param_var_idxs.append(idx) |
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.
Do you need to make sure that the Var has a value here? We allow Vars with lower == upper to be uncertain Params even if they don't have a current (numeric) value, correct?
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 do not believe we need to ensure the Var
has a value here, as:
- The nominal value of the uncertain parameter represented by the
Var
was already obtained (accounting for the state of theVar
) inutil.validate_uncertainty_specification
(see line 977 @ 7dec651) - Immediately after this loop:
- The previously obtained nominal value is used to initialize the newly constructed
Param
that is to be substituted for theVar
- The
Param
is substituted for theVar
, and (apart from cloning theVar
when the working model is cloned) theVar
is not subsequently used to set up subproblem components
- The previously obtained nominal value is used to initialize the newly constructed
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.
Great! Thanks for clarifying
…o into pyros-vars-as-params
Summary/Motivation:
Motivated by recent user feedback, this PR adds support for the speciifcation of
Var
andVarData
objects as (entries of) the argumentuncertain_params
tocontrib.pyros.PyROS.solve()
. Thus, the argumentuncertain_params
can now be aParam
,ParamData
,Var
, orVarData
instance, or an iterable sequence of such instances. AllVarData
objects derived fromuncertain_params
should be fixed by the user in advance of invoking PyROS.Note that if
uncertain_params
is of typeVar
/VarData
, or otherwise hasVar
orVarData
entries, then there may be an appreciable computational overhead during the PyROS solver preprocessing step for models with large numbers ofConstraint
,Expression
, and/orObjective
components in which theVar
/VarData
objects appear.Changes proposed in this PR:
Var
andVarData
objects inuncertain_params
argumentTODO (after #3434 merged)
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: