lmfit / asteval

minimalistic evaluator of python expression using ast module
https://lmfit.github.io/asteval
MIT License
183 stars 41 forks source link

Improve performance of make_symbol_table #106

Closed eendebakpt closed 2 years ago

eendebakpt commented 2 years ago

This is a continuation of #104. The Interpreter is constructed many times when fitting with lmfit and this calls make_symbol_table. This PR improves performance by taking many checks on the valid symbols in math, sympy and builtins out of the function.

Master:

In [2]: %timeit make_symbol_table()
172 µs ± 3.16 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

This PR:

In [59]: %timeit make_symbol_table()
11.3 µs ± 421 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Note: the (slighty) reduced coverage is in a part that was only modified by autopep and branches where numpy is not installed, but it seems numpy is installed when running coverage. If it is required to fix this, please suggest how to approach this.

codecov-commenter commented 2 years ago

Codecov Report

Merging #106 (6c6313f) into master (74bec39) will decrease coverage by 0.13%. The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
- Coverage   94.39%   94.26%   -0.14%     
==========================================
  Files           4        4              
  Lines        1446     1448       +2     
==========================================
  Hits         1365     1365              
- Misses         81       83       +2     
Impacted Files Coverage Δ
asteval/astutils.py 87.74% <79.16%> (-1.15%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 74bec39...6c6313f. Read the comment docs.

newville commented 2 years ago

@eendebakpt thanks, looks very interesting. I am traveling and may not be able to review this sufficiently for a few weeks. I am not ignoring, just may not get too quickly. Feel free to ping me again on this if I'm going too slow. Thanks again.

newville commented 2 years ago

@eendebakpt Thanks again, this looks like an impressive speedup. I am not actually concerned about the coverage report -- that's informative but does not really signal an error, especially with massive changes to code blocks.

I verified locally on one machine that all asteval tests and all lmfit tests pass with this change.

So, I will very gratefully merge this: thanks, awesome!