Consistent variable names for rings #40518
Conversation
orlitzky
force-pushed
the
consistent-variable-names
branch
2 times, most recently
from
984a010 to
a469d92
Compare
The variable_names() method is documented to return a tuple of strings, but if there are no variable names it currently raises an error. This is inconsistent with the documentation, but maybe more importantly, inconsistent with the implementation in some rings like RR where an empty tuple is returned. This commit changes the "no variables" result to the empty tuple (), with the justification that it's much easier to loop through or check for an empty tuple than it is an either-a-tuple-or-an-error.
Fix issue 34441 by removing 'x' from their lists of variables.
One test in this file is trying to inject the (nonexistent) variable names from the finitely-presented group A5, and is expecting an error. The implementation of variable_names() has been changed, however, and now returns an empty tuple rather than an error when there are no variables. As a result, A5.inject_variables() injects no variables into the session, i.e. does nothing. It seems like a weird thing to be testing in the first place, so this commit removes the test rather than updating the expected output. In another spot, the call to variable_names() is trying to catch the ValueError that we no longer throw. This has been changed to an "if", to catch the empty tuple instead.
Rather than returning an error, the variable_names() method now returns an empty tuple when there are no variable names. As a result, we have to change one try/except clause in this file to "if not".
Rather than returning an error, the variable_names() method now returns an empty tuple when there are no variable names. As a result we have to change some try/except clauses in this file to if (not).
Rather than returning an error, the variable_names() method now returns an empty tuple when there are no variable names. As a result we have to change one try/except clause in this file to "if not".
This method was added to support unpickling of older non-singleton versions of the AA object, but the test for it happens to be loading such an old version of AA into scope, making it difficult to doctest newly-fixed bugs. Since this class has been a singleton since commit dfbb8bb in 2014, I think it is time to lay this method and its test to rest.
Rather than returning an error, the variable_names() method now returns an empty tuple when there are no variable names. As a result we have to change one try/except clause in this file to if/else.
Rather than returning an error, the variable_names() method now returns an empty tuple when there are no variable names. As a result we have to change one try/except clause in this file to an "if". Note: there were two ValueErrors being caught by this one "except" clause, one from variable_names() and one from abbreviate(). Only the first becomes an "if" statement.
Rather than returning an error, the variable_names() method now returns an empty tuple when there are no variable names. As a result we have to change one try/except clause in this file to an "if".
Rather than returning an error, the variable_names() method now returns an empty tuple when there are no variable names. As a result we can drop one try/except clause in this file completely.
orlitzky
force-pushed
the
consistent-variable-names
branch
from
a469d92 to
363b6c7
Compare
|
Hi everyone who was CC'ed on #34441. I've fixed this inconsistency by returning an empty tuple rather than an error because it's usually much cleaner to loop through an empty tuple (i.e. do nothing) or check for one with The fix itself was relatively easy. Afterwards there were a few doctest failures that needed tweaks, and then a somewhat larger number of apparently untested try/except clauses around |
| sage: s = loads(b'x\x9cmQ\xcbR\x141\x14\xad\x11A\x083\xe2\x03T|' | ||
| ....: b'\x82l`\xd3\xff\xe0\x86\x8de/\xba*\xcb\xa9[\xe9\xf4' | ||
| ....: b'\xa5;e:=\'I+,\xa6J\x17B\xf9\xd7f\x08\xe2s\x95\xa4\xee9\xf7<' | ||
| ....: b'\xf2\xe5\x8e\x0e\xaa\xe5"D?\xea8z.\x9a\x0b\xa7z\xa3I[\x15' | ||
| ....: b'\x82\xf8\xf3\x85\xc9\xb1<xg[\xae\xbd2\xbabeO\r\xdb\x86>\x9b' | ||
| ....: b'\xd8\x91V\x91\xdb\xc1_\xe0f\xa57\xae\r\x05P+/\xfe\xe5\x08' | ||
| ....: b'\xaci\xa2z46\x1aG$Z\x8e*F/p\xf7oC\xa33\x18\x99</<\x07v\tf' | ||
| ....: b'\x06\'F\xe7\xb9\x195\x0b\xacg\xc2\x8d\xbc\xe1P\x9c\xad\x04' | ||
| ....: b'\x828\xcd\x076N\x96W\xb8WaSN\x17\xca\xa7\r9\r\xb6.+\x88Kl' | ||
| ....: b'\x97e\xb7\x16+LO\xbeb\xb6\xc4\xfdc)\x88\xfb\x9a\x9b&\x05' | ||
| ....: b'\xc0N)wI\x0f\xee\x13\xfbH=\xc7nh(U\xc2xP\xca\r\xd2\x8d' | ||
| ....: b'\x8a\n\x0fK\xb9\xf5+\xfe\xa3n3MV\x98\x80\xc7rr\xfe\r\xbbr' | ||
| ....: b'\x9bZv\xecU\x1c|\xc0\xde\x12O\xe4:\xd5*0\x9ev3\xb9C\x0b' | ||
| ....: b'\xa3?Z\xa6\xa4\x11R6<{?I\xa2l\xb9\xbf6;\xb8\\\xc6\xe0\xb1' | ||
| ....: b'\x9f\xb3\xf6&\xe8\xe2,\xb3R\x13\xf9\xf2\xe1\xda\x9c\xc0s' | ||
| ....: b'\xb9\xf7?.\xe1E7\xeb\xa6W\x15^&\x80q&\x1aeo\x93Y\x13"^\xcd' | ||
| ....: b'\xf1Z\xee\xdf\x92W\x18Z\xa4\xa6(\xd7\x867\xdf\x93\xad\x9fL' | ||
| ....: b'\xa5W\xff\x90\x89\x07s\x1c\xfe6\xd2\x03{\xcdy\xf4v\x8e\xa3' | ||
| ....: b'\xb1.~\x000\xc2\xe0\xa1') | ||
| sage: s is AA | ||
| True |
There was a problem hiding this comment.
I don't think you can remove this test.
There was a problem hiding this comment.
(The commit message mentions this, so apologies if you have read it already.)
The reason I've removed this test is because it is loading a copy of AA from before 2014, and AA is a singleton, so loading the old copy puts x back into the list of variables. The test for AA having no variables then of course fails. Likewise,if any other tests are added for bugfixes to AA in the future, they may mysteriously fail after the modern AA is overwritten.
I think it is pretty unlikely that someone will want to unpickle a copy of AA from before 2014, especially considering that (as evidenced by the test failures) it can bring back old bugs and incompatibilities. For a reference point, we switched to python-3.x in 2020.
FWIW I could probably hack the _names after this test to keep it working for now, but I don't feel great about loading a mysterious binary string into scope and then using it for the rest of the test suite without knowing what the differences are compare to the modern AA.
|
I don't really agree with point 2 of the PR. There is a difference between no variable names and not having assigned any. I think changing the default to be |
Fix #34441 by making the
variable_names()method forZZ,QQ, andAAreturn an empty tuple. This comes in three parts:('x',)as the list of namesValueErrorwas detected to indicate the absence of variable names. Only a small number of these actually caused doctest failures -- not terribly reassuring. The rest were caught by grepping forvariable_names()withintry:.Note: during construction,
names=Noneis usually treated as a magic value that means "no names." And in fact,names=()is not universally supported as an alternative (you'll get errors). This does not make a ton of sense to me, but I've left it alone rather than create a diff that touches 100 files.