asteval icon indicating copy to clipboard operation
asteval copied to clipboard

Improve performance of make_symbol_table

Open eendebakpt opened this issue 3 years ago • 2 comments

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.

eendebakpt avatar Jul 07 '22 12:07 eendebakpt

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.

codecov-commenter avatar Jul 07 '22 12:07 codecov-commenter

@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 avatar Jul 07 '22 19:07 newville

@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!

newville avatar Sep 01 '22 15:09 newville