Lemke refactor #209
Lemke refactor #209
Conversation
| @@ -121,7 +123,7 @@ This algorithm is implemented using integer pivoting. | |||
| In our case the first row has corresponding ratio: :math:`1/2` and the second | |||
| ratio :math:`1/1`. So our pivot row is the first row:: | |||
|
|
|||
| >>> nash.integer_pivoting.find_pivot_row(row_tableau, column_index=1) | |||
| >>> rtableau._find_pivot_row(column_index=1) | |||
There was a problem hiding this comment.
Was not aware that these methods were used in docs, can certainly make them public, though considered them more internal logic when writing code
| """ | ||
| errs = self._tableau[:, sorted(self.slack_variables)] | ||
| pivot_column = self._tableau[:, (column_index,)] | ||
| err_ratios = errs / pivot_column |
There was a problem hiding this comment.
If you compare with https://github.com/drvinceknight/Nashpy/pull/209/files#diff-afcbc2dd5fe0e1b292a6d2b26086885e45cbd461a859d4a32100904e20811b0eL50, this seems to be the fix for the bug. The original lex sort did not take into account the pivot column when lex sorting the perturbed tableau.
Beyond this had to add some handling for NaNs. Also struggled a bit getting test cases to complete. Basically its not entirely clear to me why we can do pivot_col / tableau[:,-1] in the max ratio check, while here its swapped.
tests/unit/test_lemke_howson_lex.py
Outdated
| found_eq = True | ||
| self.assertTrue(found_eq, "Did not find eq on label " + str(label)) | ||
| self.assertGreaterEqual( | ||
| nonnans, 14, msg="at least 14 eqs without nan values produced" |
There was a problem hiding this comment.
Even with lex sorting working, some of the labels generate NaNs. It would seem like this has to do with the pivoting algorithm sometimes producing high row values after many iterations and numerical instability. I haven't looked much further into this
|
Thanks a lot for this @tokheim. I'll have time over the next week or so to review. |
tests/unit/test_tableau.py
Outdated
There was a problem hiding this comment.
We should change the module doctoring here and I think I'd like to see more unit tests for each of the methods in the class (but we can do that in a separate PR).
There was a problem hiding this comment.
To make the change less scary I tried to keep the test classes as similar as possible, but do let me know if we should merge lex and regular test cases into one file etc.
I'd appreciate leaving more test cases as a separate PR, simply because I'm not sure how many high quality test cases I could provide myself.
There was a problem hiding this comment.
To make the change less scary I tried to keep the test classes as similar as possible, but do let me know if we should merge lex and regular test cases into one file etc.
Please do merge :)
I'd appreciate leaving more test cases as a separate PR, simply because I'm not sure how many high quality test cases I could provide myself.
100% I'll open an issue when we merge this PR.
|
Thanks again for the awesome work here. Looking forward to merging this contribution. |
|
|
||
|
|
||
| @deprecated( |
There was a problem hiding this comment.
We never discussed what to do with the old lex method. I left it in, with a deprecation warning in case its still used by some
It's been marking season over here so I'm coming up for air finally thanks for your patience. |
I wanted to try to find the bug that caused the infinite loop on the game I submitted, but struggled quite a bit reading the lex code. So seeing #77 I took a stab at refactoring most of the logic to make it more readable for myself. I appreciate that readability can be quite subjective, so let me know if this is too extensive, otherwise I'd be happy to make adaptations and suggestions you provide. All existing tests are passing, with just some changes to invocations. I did a crash course on lemke-howson, so not unlikely I got some of the elements wrong.
As for the actual bug, it should be possible to backport this without the refactoring. See comment in code.