SameQ[] and Expression module tweaks #1079
Conversation
Docstring comment tweak on expression_sameQ, and uncertainty comments of implementation. Some linting was done on expression module. Mathics -> Mathics3 more places.
and a few more teaks
| """ | ||
| # TODO: Consider a faster implementation. |
There was a problem hiding this comment.
It is not clear to me this is not fast enough. What is done here is equivalent to tail recursion elimination.
Actually, on my home machine I've never ran into the recursion limit problem. I guess I have more memory. The reason Python 3.12 put compiler/OS limitation to recursion was no doubt some sort of security thing. People aren't going to know (or care about) what is being referred to with "the issue".
There was a problem hiding this comment.
Actually, comparing the time the tests takes in GithubActions, it seems that this is faster than the recursive implementation.
| # causes Boxing to mess up. Untangle this mess. | ||
| if expr._cache is None: | ||
| return None | ||
|
|
There was a problem hiding this comment.
My linter is telling me correctly that expr can be uninitialized, and indeed it is the loop iterator variable in the preceding "for" loop. Testing that after the loop ends is weird. Removing this doesn't cause any test failure.
@mmatera looked the code over and do not find anything wrong with it.
I made some small changes to the docstring comment and addressed some of the lint errors my editor is telling me about.
We can't use generators here since there we access and then "rewind" sometimes. And this is essentially a version without the tail recursions. So I've removed the comments concerning doubt.
If you have the example that caused the failure on 3.12 without this change, it would be interesting to benchmark just this test inside a Mathics3 session to see if there's any difference in execution time.
Other comments I'll put in the specific code changes in this PR.