PyomoNLP scaling factors on sub-blocks#3295
Conversation
bknueven
force-pushed
the
pyomonlp-scaling
branch
from
09e3da9 to
5cf85d5
Compare
bknueven
force-pushed
the
pyomonlp-scaling
branch
from
5cf85d5 to
e7bfad9
Compare
bknueven
force-pushed
the
pyomonlp-scaling
branch
from
5e8ef5b to
c7b1bca
Compare
bknueven
changed the title
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
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.
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.
@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:
NoneProposed by this PR:
[0.01, 0.1 ]There was a problem hiding this comment.
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.
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.
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 NoneIt'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.
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
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.
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.
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.
bknueven
force-pushed
the
pyomonlp-scaling
branch
from
cd729bf to
59f001f
Compare
bknueven
force-pushed
the
pyomonlp-scaling
branch
from
59f001f to
9ca3928
Compare
This reverts commit 0b4d16b.
Thanks for the review. I believe I have addressed all comments except for potential modifications to |
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
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.
@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:
NoneProposed 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 |
bknueven
force-pushed
the
pyomonlp-scaling
branch
from
73d2ade to
031500b
Compare
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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. |
Robbybp
left a comment
Robbybp
left a comment
There was a problem hiding this comment.
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. |
5 participants
Fixes # N/A
Summary/Motivation:
Scaling factors in the
PyomoNLPinterface will not be applied if thescaling_factorSuffix is on a sub-block.Changes proposed in this PR:
PyomoNLPscaling determination methods to useSuffixFinderinstead (23d316e, a7791e1)PyomoGreyBoxNLPandPyomoNLPWithGreyBoxesto 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: