lmfit / asteval

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

🩹 Fix missing numpy rename values returned by make_symbol_table #112

Closed s-weigand closed 1 year ago

s-weigand commented 1 year ago

Hi there, I just installed the latest release (0.9.28) and found that the NUMPY_RENAMES values were missing in the symbol list created with make_symbol_table which caused a crash when creating an Interpreter with Interpreter(symtable=make_symbol_table(...)) and using renamed functions like ln.

Turned out to be a little from-to-mixup when checking that the symbol exists in numpy since the value and not the key should exists in numpy https://github.com/newville/asteval/blob/4af02f779ea4309f94d763560f522d2f8a794c32/asteval/astutils.py#L183

I also added a unitest for the case so a regression can't sneak in easily in the future.

codecov-commenter commented 1 year ago

Codecov Report

Base: 93.31% // Head: 93.40% // Increases project coverage by +0.08% :tada:

Coverage data is based on head (618e491) compared to base (4af02f7). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #112 +/- ## ========================================== + Coverage 93.31% 93.40% +0.08% ========================================== Files 4 4 Lines 1496 1500 +4 ========================================== + Hits 1396 1401 +5 + Misses 100 99 -1 ``` | [Impacted Files](https://codecov.io/gh/newville/asteval/pull/112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matt+Newville) | Coverage Δ | | |---|---|---| | [asteval/astutils.py](https://codecov.io/gh/newville/asteval/pull/112/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matt+Newville#diff-YXN0ZXZhbC9hc3R1dGlscy5weQ==) | `88.38% <100.00%> (+0.64%)` | :arrow_up: | | [tests/test\_asteval.py](https://codecov.io/gh/newville/asteval/pull/112/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matt+Newville#diff-dGVzdHMvdGVzdF9hc3RldmFsLnB5) | `97.38% <100.00%> (+0.01%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matt+Newville). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matt+Newville)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

newville commented 1 year ago

@s-weigand Thanks -- I'm not sure why I didn't see that in my tests... Was that with a specific version of numpy? Anyway, thanks!

newville commented 1 year ago

@s-weigand I don't understand the failure with Python 3.8 and no-numpy. Seems like an error with the testing setup.

s-weigand commented 1 year ago

@newville That is a very strange bug on the CI, while trying to figure out what is going on I found that either adding -vv as flag to pytest or running pytest in the repo root fixes the issue. But I still have no idea what causes it in the first place.

newville commented 1 year ago

Thanks, yeah it is weird. I'll try to merge and push a new version with this fix in the next few days.

On Fri, Nov 11, 2022 at 7:51 AM Sebastian Weigand @.***> wrote:

@newville https://github.com/newville That is a very strange bug on the CI, while trying to figure out what is going on I found that either adding -vv as flag to pytest https://github.com/s-weigand/asteval/actions/runs/3445417663 or running pytest in the repo root https://github.com/s-weigand/asteval/actions/runs/3445459416/jobs/5749195712 fixes the issue. But I still have no idea what causes it in the first place.

— Reply to this email directly, view it on GitHub https://github.com/newville/asteval/pull/112#issuecomment-1311717959, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKI66ZF6YJ27JTVKL2ZLLWHZFONANCNFSM6AAAAAAR5DF3IY . You are receiving this because you were mentioned.Message ID: @.***>

-- --Matt Newville 630-327-7411

s-weigand commented 1 year ago

@newville I changed the tests on the CI to invoke pytest with python -m pytest instead of just pytest, this also fixes this esoteric bug on python 3.8 and makes it more in line with the calls to pip.

newville commented 1 year ago

@s-weigand Thanks very much -- I agree with python -m pytest being the better approach.

s-weigand commented 1 year ago

@newville Any news about a new version that incorporates this fix?

newville commented 1 year ago

@s-weigand sorry, that slipped off my radar screen. Will push out a new version shortly.

newville commented 1 year ago

@s-weigand 0.9.29 released!