Conversation
also deprecates the use of floats in mul
so list[Expression] was not a ListLike[ExprLike]... because list is mutable, and mutable collections are type 'invariant'; what we want is 'covariant' behaviour which would treat Expression as a subclass of ExprLike https://mypy.readthedocs.io/en/stable/common_issues.html#invariance-vs-covariance The Sequence type covers tuple and list (and others, like iterators I think) and it is covariant, so that resolves that (and accepts other things we also accept, because we just use generic sequence (iterate, select element) functions on it
IgnaceBleukx
left a comment
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Overall seems fine; it's very different from our expressions, but that is the point, of course.
There is quite some duplication of handling the FloatSums in the solver interfaces; maybe it's better to move it to the _make_numexpr helper functions?
Main thing for me is some missing docs: e.g., in the template, and in the main modeling docs, making it clear FloatSum and DirectConstraint are the ONLY things where you are allowed to use floats.
A follow-up PR should probably go through all of the is_num checks and replace them with is_int instead?
| if z3.is_int_value(obj_val): | ||
| self.objective_value_ = obj_val.as_long() | ||
| elif z3.is_rational_value(obj_val): | ||
| self.objective_value_ = obj_val.numerator_as_long() / obj_val.denominator_as_long() |
|
removing the is_num's, yes, but its expansive... some are these are in the constructors that we are still cleaning up anyway, so I think after properly typing the constructors is best |
|
I refactored it in parallel... It was failing on negboolviews for some solvers, and there was also the open thing of constants. Its still a bit a non-well-typed mess: in the current form we can let the solverobject overwrite but then, the current code lets (both are not actually visible yet because our typing has not advanced that much yet) |
…atSum, add as solver capability
please re-review |
IgnaceBleukx
left a comment
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Good, almost there I think.
The main question mark for me is the objective value return.
I.e., are we sure that we will always have a float as objective value if and only if we have a FloatSum objective?
I tweaked some tests, but overall ok; docs also look fine.
| :class:`FloatSum` is **not** an Expression nor a GlobalFunction. | ||
| It is only supported by some solvers (ortools, gurobi, cplex, scip, z3, minizinc, highs, hexaly), | ||
| and must be passed directly to that solver's :meth:`~cpmpy.solvers.solver_interface.SolverInterface.minimize` | ||
| / :meth:`~cpmpy.solvers.solver_interface.SolverInterface.maximize` |
There was a problem hiding this comment.
For consistency: I think elsewhere in the code base we use :func: instead of :meth:?
There was a problem hiding this comment.
Had to look up/ask Sphinx conventions: solver minimize/maximize/objective_value are class methods of the SolverInterface class, so :meth: with full qualified paths. :func: kept only for module-level functions (decompose_in_tree, boolvar, etc.). So kept meth here (with a qualified path update)
There was a problem hiding this comment.
also update the docs/developers.md (not on readthedocs public yet)
| vars: NDVarArray | ||
| const: float | ||
|
|
||
| def __init__(self, coeffs: ListLike[float|np.floating], vars: ListLike[_NumVarImpl], const: float|np.floating = 0.0): |
There was a problem hiding this comment.
docstring for the intit function?
(I suppose just the past paragraph in the docstring of the class , but witht the "Arguments:" and "Return" thingies?)
| raise TypeError("FloatSum(coeffs, terms) expects at least one term") | ||
|
|
||
| def __repr__(self) -> str: | ||
| if self.const: |
There was a problem hiding this comment.
At first I thought this was checking for None, but its checking if it's 0, maybe add a comment?
| return None | ||
| return float(np.dot(self.coeffs, vals) + self.const) | ||
|
|
||
| def components(self, negbool=False) -> tuple[np.ndarray, NDVarArray, float]: |
There was a problem hiding this comment.
Can/should we type the NDVarArray that is returned here with _NumVarImpl?
There was a problem hiding this comment.
Also allow_negbool would be more intuitive
There was a problem hiding this comment.
allow_negbool: agreed
I don't know if there is a way to type the NDVarArray as containing _NumVarImpls? it contains objects from dtype perspective... not opening that can of worms : )
| def __mul__(self, other: object) -> NoReturn: self._raise_objective_only() | ||
| def __rmul__(self, other: object) -> NoReturn: self._raise_objective_only() | ||
| def __neg__(self) -> NoReturn: self._raise_objective_only() | ||
| def __abs__(self) -> NoReturn: self._raise_objective_only() |
There was a problem hiding this comment.
Missing a couple here: pow, truediv, floordiv, mod
https://www.pythonmorsels.com/every-dunder-method/
| return np.asarray(ws, dtype=float), cpm_array(vs), const | ||
|
|
||
| def _raise_objective_only(self) -> NoReturn: | ||
| raise TypeError("FloatSum is objective-only. Use it directly in solver minimize()/maximize().") |
There was a problem hiding this comment.
Its more specific than that, maybe: FloatSum cannot be used as a subexpression or something?
(Btw: we probably want something similar implemented for DirectConstraint)
There was a problem hiding this comment.
I'm leaving the fun of doing it for DirectConstraint to others : )
| self.objective_value_ = round(obj_val) | ||
| else: # can happen with DirectVar or when using floats as coefficients | ||
| self.objective_value_ = float(obj_val) | ||
| else: # FloatSum objective, must be read through FloatSum.value() |
There was a problem hiding this comment.
Are we sure of this? Is there no other way the solver can return a float as objective value? (e.g., due to precision issues?
I think it's safer to explicitely test if we posted a FloatSum
There was a problem hiding this comment.
right... that might happen; so we actually have to store the cpmpy object like we do for exact... refactoring
| if isinstance(expr, FloatSum): | ||
| ws, vs, const = expr.components() | ||
| self.user_vars.update(vs) # save user variables | ||
| self._obj_offset = const |
There was a problem hiding this comment.
We don't do anything with this offset in the case of floatsum, right? Its computed through the FloatSum.value function.
BUt actually, for the non-floatsum case, we should add this to the objective value after solving...
There was a problem hiding this comment.
oh, good catch. We did it in SolveAll(), but not yet in Solve(), fixed
| if round(ort_obj_val) == ort_obj_val: # its integer | ||
| self.objective_value_ = int(ort_obj_val) | ||
| else: # FloatSum objective, must be read through FloatSum.value() | ||
| self.objective_value_ = None |
There was a problem hiding this comment.
Yes so for OR-Tools its probably fine to do it like this, but for the MIP solvers we should do an extra whether we have posted a FloatSum I think
2 participants
also deprecates the use of floats in mul
if this looks good, we can apply it to the other solvers that support floats before merging (scip, highs, gurobi, cplex; maybe rc2, z3, choco, exact, hexaly?)