Conversation
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
I haven't reviewed changes in _evaluate_hessian_if_necessary_and_cache yet, but this looks fine.
Just curious, what is the motivation for doing this instead of just using outputs and putting them in the objective? Is there some motivating use case where this works better?
This is me. For applications I work with, it's common for people to just come with a black box and say "minimize". Clearly that approach is not ideal, but allowing this capability will enable us to reach out to people who are used to doing optimization that way and help us convert them to the light side of the force... |
Okay, so this is basically just for convenience.
I don't think it's clear at all. I'm usually a fan of eliminating auxiliary variables where possible, so this approach appeals to me. I was just curious if there was an example where it actually mattered for performance or reliability. |
codykarcher
left a comment
codykarcher
left a comment
There was a problem hiding this comment.
Looks good, will test functionality over the next few weeks
It is not ideal in the context of it hides structure I could potentially implement in my optimization problem. IE, what if my black box is hiding a convex optimization problem? But yea, it is a matter of convenience not performance. |
|
@michaelbynum , @Robbybp - We are hoping to cut a patch release on Monday (the 14th). Any chance this is going to make it into that? |
|
@michaelbynum , @Robbybp - Pinging on this PR again! Any update? |
pyomo/contrib/pynumero/examples/external_grey_box/external_with_objective.py
Show resolved
Hide resolved
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
Some minor comments. Otherwise, this looks good to me.
| # Support for objectives | ||
| def has_objective(self): | ||
| return False | ||
|
|
||
| def evaluate_objective(self) -> float: | ||
| """ | ||
| Compute the objective from the values set in | ||
| input_values | ||
| """ | ||
| raise NotImplementedError( | ||
| 'evaluate_objective called but not implemented in the derived class.' | ||
| ) | ||
|
|
||
| def evaluate_grad_objective(self, out=None): | ||
| """ | ||
| Compute the gradient of the objective from the | ||
| values set in input_values | ||
| """ | ||
| raise NotImplementedError( | ||
| 'evaluate_grad_objective called but not ' | ||
| 'implemented in the derived class.' | ||
| ) | ||
|
|
There was a problem hiding this comment.
Remind me: Why don't we include default methods for evaluate_hessian* that raise NotImplementedError? Is the presence of such a method used to determine Hessian support at some point?
There was a problem hiding this comment.
Looks like it:
. If we want to change that, I can create an issue.There was a problem hiding this comment.
I prefer try/except NotImplementedError to using hasattr, but I'm not sure it's important enough to change.
There was a problem hiding this comment.
I agree. I'll at least create an issue.
pyomo/contrib/pynumero/examples/external_grey_box/external_with_objective.py
Outdated
Show resolved
Hide resolved
6 participants
Summary/Motivation:
This PR adds support for objectives in PyNumero grey box models.
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: