-
-
Notifications
You must be signed in to change notification settings - Fork 654
Speed up the maxima _commands() list #40737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
The maxima _commands() method was implemented in terms of the tab-completion method, completions(), which goes one-letter-at-a-time and is very slow as a result. We reimplement it directly to call maxima's apropos() with the empty string. This gets all commands in one shot. We also clean up the list of commands a little by excluding some junk results. The diff (eliminated elements) from the old list to the new one is, {'SPLITS\\IN\\Q', 'erf_%iargs', 'exp-form', 'is-boole-eval', 'is-boole-verify', 'maybe-boole-eval', 'maybe-boole-verify', 'sstatus-aux', 'time\\/\\/call', 'unknown\\?', 'zeta%pi'} The faster method should help avoid warnings in the CI about the test being too slow.
This parameter was recently made obsolete, and as this is an internal method, we can just delete it.
3229b57
to
12a2173
Compare
Documentation preview for this PR (built with commit 5e808ca; changes) is ready! 🎉 |
It probably doesn't matter, but we should prefer the maxima library interface anywhere that performance might be an issue. We now test the _commands() method with maxima_calculus (a library interface) rather than "maxima" (a pexpect interface).
If we use a "\" instead of "-" in the list of bad characters, we catch all of the existing bad names, but get things like "SPLITS\\ IN\\ Q" as well.
The _commands() method checks the first character of each command to make sure that it starts with a letter. This _was_ searching a tuple (containing 'a' to 'Z'), but it seems to be much faster if we concatenate them all into one string and search that instead.
I added a few more small improvements, mainly to eliminate two junk names that snuck in, and to speed up the first-character check. The switch from |
Using python's walrus operator we can simplify the list comprehension that builds the list of maxima _commands(). Essentially this allows us to save the result of foo.strip() and work with that rather than the original unstripped string foo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good, thanks
cool, thank you! |
sagemathgh-40737: Speed up the maxima _commands() list Reimplement the `_commands()` method that queries maxima for a list of valid commands. Gives about a 9x speedup here, and should eliminate one of the CI "slow doctest" warnings. URL: sagemath#40737 Reported by: Michael Orlitzky Reviewer(s): Frédéric Chapoton, Michael Orlitzky
Reimplement the
_commands()
method that queries maxima for a list of valid commands. Gives about a 9x speedup here, and should eliminate one of the CI "slow doctest" warnings.