search

tulip-control / polytope

Geometric operations on polytopes of any dimension
https://pypi.org/project/polytope
Other
73 stars 19 forks source link

Reducing number of problem instances in Fourier-Motzkin projection #65

Closed krooken closed 2 years ago

krooken commented 3 years ago

Added what I think is a missing minus sign. Without it, there are multiples of some rows in the problem matrix.

murrayrm commented 3 years ago

I agree with the change. This would be a good opportunity to add a unit test that fails on the original code but succeeds on the new code, as a way of increasing unit test coverage (which right now is quite poor).

@krooken Would you have time to do that? If not, we can merge as is, I think.

krooken commented 3 years ago

I'll take a look. I can spend some time, but I found it through inspection, so I am not sure if I can find a failing case. I think it has more to do with performance and less with correctness.

murrayrm commented 3 years ago

In that case, maybe just create some unit tests that cover the code and don't worry about something that is different for the prior version.

krooken commented 3 years ago

Sure, I'll see what I can do.

krooken commented 3 years ago

I've added some tests, so this is ready for review now.

slivingston commented 2 years ago

@krooken The square and triangle test cases in R^2 are good: easy to visualize and minimal in terms of number of dimensions.

slivingston commented 2 years ago

While merging this, I renamed the tests to have the style of "test_" prefix instead of "_test" suffix.