Conversation
|
test models please |
whoburg
left a comment
whoburg
left a comment
There was a problem hiding this comment.
Thanks for doing it right!
Please address inline comments before merging.
| self.assertAlmostEqual(senss[R], 0.41, 2) | ||
| self.assertAlmostEqual(senss[fuel_per_nm], 0.41, 2) | ||
| self.assertAlmostEqual(senss[W_payload], 0.39, 2) | ||
|
|
| [x >= 1, | ||
| y == 2]) | ||
| m.solve(verbosity=0) | ||
| m.solve(solver=self.solver, verbosity=0) |
| for exp in zeroed_terms: | ||
| del matches[exp] | ||
| if not matches: # it's a zero monomial | ||
| matches[HashVector()] = 0 |
There was a problem hiding this comment.
interesting, why do we need to set matches[HashVector()] = 0 here? I would have thought we just let exps_ = (), and cs = [] (and therefore mmap = [] if returned) in that case.
There was a problem hiding this comment.
Not in the current scheme; the current abstraction for simplification is Bad, but this wasn't the PR to fix it...
| exps_.pop(i) | ||
| exps = tuple(exps_) | ||
| if pmap is not None: | ||
| self.const_mmap = pmap.pop(i) # pylint: disable=attribute-defined-outside-init |
There was a problem hiding this comment.
whoa, this mutates pmap? That ought to be documented.
There was a problem hiding this comment.
and is that really intended?
| self._last_used_substitutions = subs | ||
| exps, cs = self._simplify_posy_ineq(exps, cs) | ||
| exps, cs, pmap = simplify_exps_and_cs(exps, cs, return_map=True) | ||
| exps, cs, pmap = self._simplify_posy_ineq(exps, cs, pmap) |
There was a problem hiding this comment.
There's a lot going on in the four lines above (substitution, and two simplification steps). Can you add an inline comment to clarify what the two different simplification steps are? Or perhaps make the method names more descriptive?
It's particularly interesting to me that the two simplification steps switched order in this PR -- what's up with that?
There was a problem hiding this comment.
That's what most of the work of this PR goes into, is designing self._simplify_posy_ineq so that it allows that. Thus we end up with an mmap for the constant term that can be noted during constraint simplification.
|
Added comments per review. |
2 participants
Closes #1060
@whoburg, ready for review.