BUG: amos/amos.h: fix scope of l in seri#92
Conversation
Fix the scope of l such that its value from the loop is conserved after the goto statement in the loop's body.
JaRoSchm
changed the title
l in seri
|
I added a test to this PR which fails on main and passes here. In this case, I chose not to include 260 reference values but simply compare the "vectorized order" version to the "scalar" one. The "scalar" version is tested separately and should be fine as a reference. Additionally, I now use "test cases", which will allow me to simply add another test for #93, when this is merged. |
| } | ||
| } | ||
|
|
||
| TEST_CASE("amos besh vectorized", "[amos][xsf_tests]") { |
There was a problem hiding this comment.
This test seems useful but should we not add a sentinel regression test as you did #158? It passes on main.
There was a problem hiding this comment.
Thanks for checking! Seems like I have mixed up what this PR actually fixes with some other PR. I have now added a test similar to #153 here.
| int l; | ||
| for (l = 3; l < (nn + 1); l++) { |
There was a problem hiding this comment.
Indeed, this seems to fix the translation error.
| } | ||
| } | ||
|
|
||
| TEST_CASE("amos seri buffer overflow gh-92", "[amos][xsf_tests]") { |
There was a problem hiding this comment.
I tried to run this test on upstream/main and it still passes.
There was a problem hiding this comment.
Hm, I tried again and for me the test fails if I revert the fix. I reverted the fix and pushed to see what CI says.
There was a problem hiding this comment.
Okay, looks like you are using macOS? Then CI agrees with you. On Linux CI reproduces the failure I observe. So the effect of the bug might be ill-defined in C++ and gcc/clang give different results. Could this be the origin of the differences?
Would you be willing to merge if I revert the last commit? Looks like we agree that the fix is correct and we have a regression test at least for some of the CI jobs.
There was a problem hiding this comment.
I think those are memory bugs. I would expect CI to fail for all three Operating Systems.
Co-authored-by: Florian Bourgey <bourgeyflorian@gmail.com>
|
CI failed successfully, see https://github.com/scipy/xsf/pull/92/checks?sha=8a3cb53b1425a2de1d73dd67fe959bc6513ec017. I put the fix back in. I will update the test for #93 accordingly. |
|
Thanks! |
|
Thank you for the review and for improving the test! |
2 participants
Hi, while in the long term I am still interested of getting #50 into xsf and scipy, I am using my own Python wrapper around your C translation of AMOS in the meantime. I noticed that for some parameters the implementation of
beshtries to write the result intocy[-1]. To find the origin of this I compared the xsf translation to the automatic AMOS translation from https://github.com/jpcima/zbessel. The wrapper I used for this can be found at https://github.com/JaRoSchm/pyamos. There I added a check which allows me to detect if AMOS tries to write out of the range of the result array, see https://github.com/JaRoSchm/pyamos/blob/1cd435a33db41d3bad2448991ca0e5027a87057e/pyamos.cpp#L44C1-L50C10. To see the problem, you can simply runpixi run python compare_xsf_zbessel.pythere.The bug I found using this is the scope of the variable
lin theserifunction (see the diff of this PR). After thegoto L100statement (line 4494), the value oflfrom the loop is needed and not the valuel = 3assigned beforehand. Without the change from this PR, we hadl = 3in line 4499, which is not the wanted value. The original Fortran code can be found at https://github.com/scipy/scipy/blob/4edfcaa3ce8a387450b6efce968572def71be089/scipy/special/amos/zseri.f#L147.I thought about adding a test for this, but I am not sure, how to do this. In https://github.com/JaRoSchm/pyamos/blob/main/compare_xsf_zbessel.py I use
n = 260resulting in a array of this length, which would have to be added as a reference value. Additionally, I actually never used C++ (and its test libraries), so all of this is an extrapolation of my Python and C knowledge.