d-krupke / cpsat-primer

The CP-SAT Primer: Using and Understanding Google OR-Tools' CP-SAT Solver
https://d-krupke.github.io/cpsat-primer/
Creative Commons Attribution 4.0 International
292 stars 27 forks source link

Feat/test snippets #29

Closed ngoc2210 closed 1 week ago

ngoc2210 commented 3 months ago

I have currently made some test cases for some snippets. Can you please give me some feedback about it?

d-krupke commented 3 months ago
  1. You don't need any fixtures. Just put the whole model into a single test.
  2. Make it a complete test with Solve(). Value will not return a sensible result if you haven't solved the model.
  3. You do not need to test CP-SAT, just the API. So checking proto is not necessary.

Here an example for a good test case:

def test_simple_intvar_example():
   model = cp_model.CpModel()

   x = model.NewIntVar(0, 100, "x")
   y = model.NewIntVar(0, 100, "y")

   model.Add(x + y <= 30)
   model.Maximize(30 * x + 50 * y)

   solver = cp_model.CpSolver()
   status = solver.Solve(model)
   assert status == cp_model.OPTIMAL, "Should be optimal"

   assert solver.Value(x) == 0, "x is expected to be 0"
   assert solver.Value(y) == 30, "y is expected to be 30"
d-krupke commented 3 months ago

And you can split the tests into multiple files when they are not related.

ngoc2210 commented 3 months ago

thanks a lot. I've just pushed another test case based on your example. I hope it would be the correct one.

d-krupke commented 3 months ago

Yes, this would actually be a useful test case. However, you can keep it much simpler. The primary purpose is to verify that the interface of CP-SAT is used correctly in the examples. Thus, you do not need to do anything complicated. You can really just extend the examples of the primer by model and solve such that you can verify there is no error in the usage of the interface.

d-krupke commented 3 months ago

Here kind of errors I want to catch with the tests:

ngoc2210 commented 3 months ago

many thanks for your feedbacks :)) I pushed two commits to check the naming of arguments in methods NewIntervalVar and NewFixedSizedIntervalVar. I also found that the name of the method to create interval variable with fixed length is NewFixedSizedIntervalVar, instead of NewIntervalVar in README file.

ngoc2210 commented 3 months ago

I also pushed the test for the three constraints model.AddMultiplicationEquality(xyz, [x, y, z]) # xyz = x*y*z, model.AddModuloEquality(x, y, 3) # x = y % 3, model.AddDivisionEquality(x, y, z) # x = y // z, maybe I now understand what you mean the feasible solutions for the given constraints exist but the tool might not be able to find them and you want to observe if it can find the solutions in the future. Is this right?

d-krupke commented 3 months ago

I also pushed the test for the three constraints model.AddMultiplicationEquality(xyz, [x, y, z]) # xyz = x*y*z, model.AddModuloEquality(x, y, 3) # x = y % 3, model.AddDivisionEquality(x, y, z) # x = y // z, maybe I now understand what you mean the feasible solutions for the given constraints exist but the tool might not be able to find them and you want to observe if it can find the solutions in the future. Is this right?

For these simple instances, CP-SAT should always be able to find the solution. The actual problem that can arise is that the interface changes or gets restricted. For example, AddMultiplicationEquality changes to the same interface as AddDivisionEquality were instead of a list, I have to pass a,b,c. In this case, the test would throw an exception and I know that there has been a breaking change.

CP-SAT is not necessarily a stable software, yet. Thus, things change with every new version. I need to get automatically informed about this so I can update the primer. I will get automatically notified if a test fails for the latest version of CP-SAT by automatically executing them via the GitHub CI.

d-krupke commented 3 months ago

This has happened in the past, which is why I want tests :)

d-krupke commented 3 months ago

I like the test_snippets/test_FixedSizedIntervalvar.py test :)

ngoc2210 commented 3 months ago

I have changed model.AddMultiplicationEquality(xyz, [x, y, z]) to model.AddMultiplicationEquality(xyz, x, y, z). Is this the same what you mean?

d-krupke commented 3 months ago

I have changed model.AddMultiplicationEquality(xyz, [x, y, z]) to model.AddMultiplicationEquality(xyz, x, y, z). Is this the same what you mean?

I don't know. Does CP-SAT support this syntax now? (I do not use this constraint in my daily work as it is super inefficient)

ngoc2210 commented 3 months ago

no, it still doesn't work. I changed (AddMultiplicationEquality(xyz, [x, y, z]) to AddMultiplicationEquality(xyz, x, y, z) because I thought that is what you mean here "For example, AddMultiplicationEquality changes to the same interface as AddDivisionEquality were instead of a list, I have to pass a,b,c. ".

d-krupke commented 3 months ago

no, it still doesn't work. I changed (AddMultiplicationEquality(xyz, [x, y, z]) to AddMultiplicationEquality(xyz, x, y, z) because I thought that is what you mean here "For example, AddMultiplicationEquality changes to the same interface as AddDivisionEquality were instead of a list, I have to pass a,b,c. ".

Maybe I should have use a more explicit conjunctive here: "If it were to change". This is just a wild example of what could happen.

ngoc2210 commented 3 months ago

ohh now i remember a little bit about english grammar, what i almost forgot haha

d-krupke commented 1 week ago

I have already integrated a lot of the tests. Closing.