Improve example testing #651
Conversation
Seems to take very long (but not on master?)
Unlike pytest-xdist, the user *needs* timeouts to run the test-suite completely
|
Following tests are now newly failing for a TO of 1 minute, running with or-tools and minizinc (gurobi is set in the test, but not installed in CI, so is skipped): Some examples get all solutions as default, I will lower that, which might fix some timeouts. Note the CI takes now 27 minutes rather than the 22 minutes on main. |
Probably most of these are just problems in the examples themselves, which I can probably fix in this PR, and the >60.0s ones we can see if it is intentional that they take so long, and if so, add them to the example ignore list in the test. If I find anything substantial I'll make an issue. |
|
Nice job! From a glance it also seems the examples are much improved, also in style / simpler constraints, but I don't know them that well. I'll add a few small comments Quick comment for tias on what is run with this new, it is similar to the old one which also ran three solvers (or-tools, minizinc, gurobi), if they are installed, otherwise skip. Note gurobi is not installed on the CI, we might want to look into adding that, and seeing if it is still so slow now that the examples are improved (and solutions limited in many cases). Also I see the CI only takes 20 minutes now, even less than before adding more examples (which might be slightly strange), but anyway I suggest keeping both solvers on. Here is one more to clean up if you don't have exact installed: |
| x_d_int = x_d.astype(int) | ||
| x_0_val_int = x_0.value().astype(int) | ||
| # Add constraint using integer coefficients | ||
| master_model += [sum(d * x_d_int) >= sum(d * x_0_val_int)] |
There was a problem hiding this comment.
I'm surprised these casts are needed?
There was a problem hiding this comment.
Without the casts, this error is thrown in the master_model solving (OR-tools):
TypeError: Not a number: False of type <class 'numpy.bool_'>
| @@ -209,7 +208,7 @@ def explain_one_step_ocus(hard, soft_lit, cost, remaining_sol_to_explain, solver | |||
| print("\n\t hs =", hs, S) | |||
|
|
|||
| # SAT check and computation of model | |||
| if not SAT.solve(assumptions=S): | |||
| if not SAT.solve(assumptions=list(S)): | |||
There was a problem hiding this comment.
why is this needed, to wrap S?
There was a problem hiding this comment.
In the or-tools interface, the solver_var method contains this line: if cpm_var not in self._varmap:, which means that if cpm_var is not hashable then there is a TypeError raised. So, this wrapping is needed to convert the set to a list before solving. This is the error raised if not wrapped:
File ".../cpmpy/cpmpy/solvers/ortools.py", line 293, in solver_var
if cpm_var not in self._varmap:
TypeError: unhashable type: 'set'
| # return soft weight if constraint is a soft constraint | ||
| if len(set({cons}) & set(soft)) > 0: | ||
| # return soft weight if the constraint is a soft constraint | ||
| if len({cons} & set(soft)) > 0: |
There was a problem hiding this comment.
this is odd code... if cons in set(soft)?
| (config[d+1, t] == DUKE) & (where[d+1,t] == AWAY)) | ||
| model += ~((config[d, t] == DUKE) & (where[d,t] == AWAY) & | ||
| (config[d+1, t] == UNC) & (where[d+1,t] == AWAY)) | ||
| model += ((config[d, t] == UNC) & (where[d, t] == AWAY) & |
There was a problem hiding this comment.
your code change removes the ~, the not...
and it replaces it by 'implies(False)', which I find MUCH more unintuitive then just negating the statement!? why would this be better?
There was a problem hiding this comment.
This change fixes an error with the previous version of the constraints. But it is indeed unintuitive, so I changed it back to the earlier version, while keeping the fix of the error.
| for boat in range(n_boats): | ||
| model += (is_host[boat]).implies(all(visits[:,boat] == boat)) | ||
| model += (is_host[boat]).implies((visits[:, boat] == boat).all()) |
There was a problem hiding this comment.
is there a reason to prefer .all() over cp.all(...)? I prefer a cp.all() upfront...
| for slot in range(n_periods): | ||
| for boat in range(n_boats): | ||
| model += sum((visits[slot] == boat) * crew_size) <= capacity[boat] | ||
| model += sum((visits[slot] == boat) * crew_size) + crew_size[boat] * is_host[boat] <= capacity[boat] |
There was a problem hiding this comment.
was the previous code wrong? the comment says 'number of visitors', so without the crew...? and why does being the host matters for the crew size?
There was a problem hiding this comment.
Yes, the constraint was missing the host boat's crew, which is exactly what is added here. The comment was misleading (I corrected it now) as the original problem description specifies that "The total number of people aboard a boat, including the host crew and guest crews, must not exceed the capacity".
| from cpmpy.exceptions import NotSupportedError, TransformationNotImplementedError | ||
| import itertools | ||
|
|
||
| prefix = '.' if 'y' in getcwd()[-2:] else '..' |
There was a problem hiding this comment.
I am not sure about this one (it was already there). From what it seems, it configures the relative path of the examples depending on whether the current working directory ends with something that contains 'y'?
I think the examples are great, and that it is important to have them work in case someone tries them out. This PR improves the example testing script:
__main__set, otherwise a lot of examples are skipped..[test], namelypytest-timeout