MAINT: remove Fortran like handling of constants#132
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
dschmitz89
changed the title
Co-authored-by: Copilot <copilot@github.com>
| // TOL WHERE B IS THE BASE OF THE ARITHMETIC. | ||
| // | ||
| t1 = (i1mach[13] - 1) * d1mach[4] * (std::log(10) / std::log(2)); | ||
| t1 = (std::numeric_limits<double>::digits - 1) * LOG10_2 * (std::log(10) / std::log(2)); |
There was a problem hiding this comment.
Can we rewrite this as
| t1 = (std::numeric_limits<double>::digits - 1) * LOG10_2 * (std::log(10) / std::log(2)); | |
| t1 = std::numeric_limits<double>::digits - 1; |
Co-authored-by: Florian Bourgey <bourgeyflorian@gmail.com>
|
@JaRoSchm Since you are familiar with AMOS, could you have a final look here? My long term plan was to make this more maintainable by using modern idioms and removing gotos which are planned for follow ups. |
JaRoSchm
left a comment
JaRoSchm
left a comment
There was a problem hiding this comment.
In general, looks like a nice improvement!
| std::complex<double> zeta2[2] = {0.0}; | ||
| std::complex<double> cy[2] = {0.0}; | ||
| double bry[3] = {1e3 * d1mach[0] / tol, tol / 1e3 * d1mach[0], d1mach[1]}; | ||
| double bry[3] = {THRESHOLD_MIN / tol, tol / THRESHOLD_MIN, std::numeric_limits<double>::max()}; |
There was a problem hiding this comment.
This fixes a translation bug, see https://github.com/scipy/scipy/blob/4edfcaa3ce8a387450b6efce968572def71be089/scipy/special/amos/zunk1.f#L46.
There's definitely potential for that in amos! In my opinion, it might be worth however to first try to fix #137 (fixing long known bugs/accuracy improvements in the Fortran implementation by the original author, which were not included in scipy before the translation) as this probably becomes harder if the versions diverge even more (and they already have quite a bit). In principle, I plan to look into this myself, but I can't say exactly when I'll get round to it yet. |
|
@JaRoSchm Thanks. Feel free to ping me when you feel like this should be merged. You have by far the best overview of everything amos related here and we should follow the PR order you propose. I will wait with resolving the merge conflicts until then. |
|
@dschmitz89 I see no reason for preferring any specific order of merging the open amos-related PRs (only #92 should be merged before I can add a test to #93). My previous comment is addressed by #149. |
JaRoSchm
left a comment
JaRoSchm
left a comment
There was a problem hiding this comment.
By chance, I stumbled across two more translation errors.
|
@JaRoSchm Would you be willing to make a PR against my branch? I am starting to lose track of all the AMOS changes and this code is very hard to digest mentally. If not, we could still apply this change and proceed in a followup. |
|
@dschmitz89 I think you could simply accept @fbourgey's and my suggestions here (should be possible through the GitHub UI?). Then, this is ready to merge from my pov and #132 (comment) could be left for a followup. |
Co-authored-by: JaRoSchm <jaro.schmidt@gmail.com>
| // | ||
| az = std::abs(z); | ||
| bb = d1mach[1] * 0.5; | ||
| bb = std::numeric_limits<int>::max() * 0.5; |
There was a problem hiding this comment.
From https://github.com/scipy/xsf/blob/main/include/xsf/amos/amos.h#L141, should this be?
| bb = std::numeric_limits<int>::max() * 0.5; | |
| bb = std::numeric_limits<double>::max() * 0.5; |
There was a problem hiding this comment.
This fixes a translation bug, see #132 (review).
|
|
||
| aa = 0.5 / tol; | ||
| bb = d1mach[1] * 0.5; | ||
| bb = std::numeric_limits<int>::max() * 0.5; |
There was a problem hiding this comment.
Same here
| bb = std::numeric_limits<int>::max() * 0.5; | |
| bb = std::numeric_limits<double>::max() * 0.5; |
There was a problem hiding this comment.
This fixes a translation bug, see #132 (review).
|
@dschmitz89 can you please resolve the conflict? I can then take a fresh look, and get that one in if OK. |
3 participants
This is a first step to make AMOS speak a little less FORTRAN.