-
Notifications
You must be signed in to change notification settings - Fork 505
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
PyomoNLP scaling factors on sub-blocks #3295
Conversation
09e3da9
to
5cf85d5
Compare
5cf85d5
to
e7bfad9
Compare
5e8ef5b
to
c7b1bca
Compare
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.
Thanks for the PR, this is a good change. I have a minor concern about backward incompatibility, see below. Also, can you add a small unit test to TestPyomoNLP
in test_nlp.py
that exercises this functionality?
scaling_suffix_finder = SuffixFinder('scaling_factor') | ||
for i, v in enumerate(self._pyomo_model_var_datas): | ||
v_scaling = scaling_suffix_finder.find(v) | ||
if v_scaling is not None: | ||
need_scaling = True | ||
self._primals_scaling[i] = v_scaling |
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 a minor edge case, but should we enforce that we never use a suffix that is "outside the scope" (above) the pyomo_model
that was used to construct the PyomoNLP
? Maybe SuffixFinder
should support a context
argument.
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.
@blnicho @jsiirola @michaelbynum Can I get a second opinion here? The question is: what should happen when a PyomoNLP is constructed from a subblock that has relevant scaling factors on the parent block:
import pyomo.environ as pyo
from pyomo.contrib.pynumero.interfaces.pyomo_nlp import PyomoNLP
m = pyo.ConcreteModel()
m.b = pyo.Block()
m.b.x = pyo.Var([1, 2], initialize={1: 100, 2: 20})
# Components so we don't have an empty NLP
m.b.eq = pyo.Constraint(expr=m.b.x[1]*m.b.x[2] == 2000)
m.b.obj = pyo.Objective(expr=m.b.x[1]**2 + m.b.x[2]**2)
m.scaling_factor = pyo.Suffix(direction=pyo.Suffix.EXPORT)
m.scaling_factor[m.b.x[1]] = 1e-2
m.scaling_factor[m.b.x[2]] = 1e-1
nlp = PyomoNLP(m.b)
scaling = nlp.get_primals_scaling()
print(scaling)
Current behavior:
None
Proposed by this PR:
[0.01, 0.1 ]
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 think that I would advocate the old behavior (although I can see value in both approaches!).
My rationale is the following: if we define a "model" as all "active components reachable through active blocks contained within the reference block," then we should exclude Suffixes outside of the subtree, as Suffix is an active component.
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'm inclined to agree.
pyomo/contrib/pynumero/algorithms/solvers/tests/test_cyipopt_interfaces.py
Outdated
Show resolved
Hide resolved
scaling_suffix_finder = SuffixFinder('scaling_factor', 1.0) | ||
primals_scaling = np.ones(self.n_primals()) | ||
for i, v in enumerate(self.get_pyomo_variables()): | ||
primals_scaling[i] = scaling_suffix_finder.find(v) | ||
return primals_scaling |
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.
Returning ones instead of None
here when no scaling factors have been defined is the backward incompatibility I'm concerned about. Maybe something like this would be better?
scaling_suffix_finder = SuffixFinder("scaling_factor")
scaling_factors = [scaling_suffix_finder.find(v) for v in self.get_pyomo_variables()]
has_scaling_factors = any(sf is not None for sf in scaling_factors)
if has_scaling_factors:
return [sf if sf is not None else 1.0 for sf in scaling_factors]
else:
return None
It's not exactly the same as having any non-empty scaling_factor
suffix used to be enough to avoid returning None
. Now we need to actually contain one of the primal variables. Maybe we could make the SuffixFinder._get_suffix_list
method public, and return None
if all these suffixes are empty?
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 implemented something equivalent originally (see 23d316e) but this will break the existing "hacks" in the implicit function solver:
pyomo/pyomo/contrib/pynumero/algorithms/solvers/implicit_functions.py
Lines 406 to 410 in ae354ae
# I need scaling_factor so Pyomo NLPs I create from these blocks | |
# don't break when ProjectedNLP calls get_primals_scaling | |
block.scaling_factor = Suffix(direction=Suffix.EXPORT) | |
# HACK: scaling_factor just needs to be nonempty | |
block.scaling_factor[block._obj] = 1.0 |
To support both cases this function could an optional argument always_return_numeric=False
or something similar to determine which behavior is desired. Maybe you have a better idea about how to de-conflict?
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, if we do this, I'll have to explicitly set scaling factors to 1 in the implicit function solver, which wouldn't be the worst thing ever. I think explicitly checking SuffixFinder._get_suffix_list
for at least one non-empty suffix would get around this as well.
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.
In 9ca3928 I have opted just to replicate the original check. This seems the safest and doesn't involve accessing private methods or modifying other parts of Pyomo.
cd729bf
to
59f001f
Compare
59f001f
to
9ca3928
Compare
This reverts commit 0b4d16b.
Thanks for the review. I believe I have addressed all comments except for potential modifications to |
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.
Thanks for making this backwards compatible. That part looks good. Just want to get a second opinion about suffixes on parent components.
scaling_suffix_finder = SuffixFinder('scaling_factor') | ||
for i, v in enumerate(self._pyomo_model_var_datas): | ||
v_scaling = scaling_suffix_finder.find(v) | ||
if v_scaling is not None: | ||
need_scaling = True | ||
self._primals_scaling[i] = v_scaling |
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.
@blnicho @jsiirola @michaelbynum Can I get a second opinion here? The question is: what should happen when a PyomoNLP is constructed from a subblock that has relevant scaling factors on the parent block:
import pyomo.environ as pyo
from pyomo.contrib.pynumero.interfaces.pyomo_nlp import PyomoNLP
m = pyo.ConcreteModel()
m.b = pyo.Block()
m.b.x = pyo.Var([1, 2], initialize={1: 100, 2: 20})
# Components so we don't have an empty NLP
m.b.eq = pyo.Constraint(expr=m.b.x[1]*m.b.x[2] == 2000)
m.b.obj = pyo.Objective(expr=m.b.x[1]**2 + m.b.x[2]**2)
m.scaling_factor = pyo.Suffix(direction=pyo.Suffix.EXPORT)
m.scaling_factor[m.b.x[1]] = 1e-2
m.scaling_factor[m.b.x[2]] = 1e-1
nlp = PyomoNLP(m.b)
scaling = nlp.get_primals_scaling()
print(scaling)
Current behavior:
None
Proposed by this PR:
[0.01, 0.1 ]
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3295 +/- ##
==========================================
- Coverage 88.52% 88.52% -0.01%
==========================================
Files 868 868
Lines 98398 98413 +15
==========================================
+ Hits 87109 87116 +7
- Misses 11289 11297 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@bknueven the PR adding the |
73d2ade
to
031500b
Compare
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 fine, but there is a cleaner / more efficient way to make use of the SuffixFinder
val = SuffixFinder('scaling_factor', context=self._pyomo_model).find(obj) | ||
# maintain backwards compatibility | ||
scaling_suffix = self._pyomo_model.component('scaling_factor') | ||
if scaling_suffix and scaling_suffix.ctype is pyo.Suffix: | ||
if obj in scaling_suffix: | ||
return scaling_suffix[obj] | ||
return 1.0 | ||
return None | ||
return 1.0 if val is None else val | ||
else: | ||
return val |
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 fine, but could be significantly simpler:
scaling_finder = SuffixFinder('scaling_factor', default=1.0, context=self._pyomo_model)
val = suffix_finder.find(self.get_pyomo_objective())
if not scaling_finder.all_suffixes:
return None
return val
def get_primals_scaling(self): | ||
scaling_suffix_finder = SuffixFinder( | ||
'scaling_factor', context=self._pyomo_model | ||
) | ||
primals_scaling = np.ones(self.n_primals()) | ||
ret = None | ||
for i, v in enumerate(self.get_pyomo_variables()): | ||
val = scaling_suffix_finder.find(v) | ||
if val is not None: | ||
primals_scaling[i] = val | ||
ret = primals_scaling | ||
# maintain backwards compatibility | ||
scaling_suffix = self._pyomo_model.component('scaling_factor') | ||
if scaling_suffix and scaling_suffix.ctype is pyo.Suffix: | ||
primals_scaling = np.ones(self.n_primals()) | ||
for i, v in enumerate(self.get_pyomo_variables()): | ||
if v in scaling_suffix: | ||
primals_scaling[i] = scaling_suffix[v] | ||
return primals_scaling | ||
return None | ||
else: | ||
return ret |
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 fine, but could be significantly simpler:
scaling_finder = SuffixFinder('scaling_factor', default=1.0, context=self._pyomo_model)
primals_scaling = numpy.fromiter(
(scaling_finder.find(v) for v in self.get_pyomo_variables()),
count=self.n_primals(),
)
if not scaling_finder.all_suffixes:
return None
return primals_scaling
def get_constraints_scaling(self): | ||
scaling_suffix_finder = SuffixFinder( | ||
'scaling_factor', context=self._pyomo_model | ||
) | ||
constraints_scaling = np.ones(self.n_constraints()) | ||
ret = None | ||
for i, c in enumerate(self.get_pyomo_constraints()): | ||
val = scaling_suffix_finder.find(c) | ||
if val is not None: | ||
constraints_scaling[i] = val | ||
ret = constraints_scaling | ||
# maintain backwards compatibility | ||
scaling_suffix = self._pyomo_model.component('scaling_factor') | ||
if scaling_suffix and scaling_suffix.ctype is pyo.Suffix: | ||
constraints_scaling = np.ones(self.n_constraints()) | ||
for i, c in enumerate(self.get_pyomo_constraints()): | ||
if c in scaling_suffix: | ||
constraints_scaling[i] = scaling_suffix[c] | ||
return constraints_scaling | ||
return None | ||
else: | ||
return ret |
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 fine, but could be significantly simpler:
scaling_finder = SuffixFinder('scaling_factor', default=1.0, context=self._pyomo_model)
constraints_scaling = numpy.fromiter(
(scaling_finder.find(v) for v in self.get_pyomo_constraints()),
count=self.n_constriaints(),
)
if not scaling_finder.all_suffixes:
return None
return constraints_scaling
self._primals_scaling = np.ones(self.n_primals()) | ||
scaling_suffix = self._pyomo_nlp._pyomo_model.component('scaling_factor') | ||
scaling_suffix_finder = SuffixFinder( | ||
'scaling_factor', context=self._pyomo_model | ||
) | ||
for i, v in enumerate(self.get_pyomo_variables()): | ||
v_scaling = scaling_suffix_finder.find(v) | ||
if v_scaling is not None: | ||
need_scaling = True | ||
self._primals_scaling[i] = v_scaling | ||
# maintain backwards compatibility | ||
scaling_suffix = self._pyomo_model.component('scaling_factor') | ||
if scaling_suffix and scaling_suffix.ctype is pyo.Suffix: | ||
need_scaling = True | ||
for i, v in enumerate(self.get_pyomo_variables()): | ||
if v in scaling_suffix: | ||
self._primals_scaling[i] = scaling_suffix[v] |
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 fine, but could be significantly simpler:
scaling_finder = SuffixFinder('scaling_factor', default=1.0, context=self._pyomo_model)
self._primals_scaling = numpy.fromiter(
(scaling_finder.find(v) for v in self.get_pyomo_variables()),
count=self.n_primals(),
)
need_scaling = bool(scaling_finder.all_suffixes)
Thanks for showing me that; I implemented your suggestion. |
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 you add a test that makes sure we don't pull a scaling factor from a parent block?
Sure, I just copy+pasted your example above and made it a test in 7e79e19. |
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.
Thanks, looks good.
Fixes # N/A
Summary/Motivation:
Scaling factors in the
PyomoNLP
interface will not be applied if thescaling_factor
Suffix is on a sub-block.Changes proposed in this PR:
PyomoNLP
scaling determination methods to useSuffixFinder
instead (23d316e, a7791e1)PyomoGreyBoxNLP
andPyomoNLPWithGreyBoxes
to useSuffixFinder
(8215ef8, c7b1bca)scaling_factors
(0b4d16b)As-is this PR would change the behavior of these methods in that they would always return scaling factors. I cannot seem to exactly replicate the prior behavior which would allow all existing tests to pass without modification.
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: