Closed jacobwilliams closed 2 years ago
Thanks for working on this!
Note: this refactor is coming along slowly but surely. I've now also got a script that pulls down and compiles the original minpack code with the original test cases. I'm going to use that as an initial cut to address #32 (i.e., just make sure we get the same answers as before...although due to bug fixes maybe some will be different).
All results identical except for the following (which fail in both original and new version):
Original:
2 4 134 7 4 0.1552863D-34
2 4 139 10 4 0.7338626D-34
2 4 149 10 4 0.4326098D-33
New:
2 4 105 6 4 0.6046010D-34
2 4 124 6 4 0.5013400D-34
2 4 143 13 4 0.2112179D-33
note: these differences are all due to the slightly different values of dpmpar from the intrinsics, compared to the old hard-coded values, which were not full double precision
Very small differences in almost all the cases. The only appreciable difference (in a failed case) is:
Old:
7 7 657 4 0.2416003D+00
New:
7 7 180 4 0.2491665D+15
note: these differences are all due to the slightly different values of dpmpar
from the intrinsics, compared to the old hard-coded values, which were not full double precision
All cases identical.
All cases identical, except for one where the original case results in an NaN:
Old:
16 40 40 2 2 4 NaN
New:
16 40 40 19 14 2 0.2045771D-12
TODO: look into this one... this one does not appear to be caused by dpmpar
Mostly identical, with some very small differences in some cases.
note: these differences are all due to the slightly different values of dpmpar from the intrinsics, compared to the old hard-coded values, which were not full double precision
All cases identical, except for two with very small differences. note: these differences are all due to the slightly different values of dpmpar from the intrinsics, compared to the old hard-coded values, which were not full double precision
OK, except for that one NaN issues in test_lmstr
(which was perhaps a bug fixed by #7), the refactor now gives the same answers as before...except for that we changed the machine constants. I don't consider that a real problem (the ones we are using now are correct, whereas the old ones aren't quite).
Next step is adding the "truth" values to the tests so they are checked in the CI. Probably with some tolerance?
I think this is ready to merge. This is just an initial version of some checks. What this is doing is checking that the results are the same as the original minpack (to within some tolerance). The code does seem to be somewhat sensitive to different compilers and settings. The lm*
ones moreso than the hybr*
ones. There are also cases where the original minpack failed (info>4
) and in those cases, different platforms will give much different answers, so I'm just excluding those cases from the pass/fail test.
There is more that can be done here, but I think this is a good start, just to get something in master than is checking the results. We can add to it over time.
The
vecfcn
,vecjac
,initpt
functions are duplicated for each test, it would be better to define them in a single module and use them for all tests, this way further refactoring doesn't need to change test files again.
Yes, agree. There are other duplications (e.g., the constants). We can move them all into a separate module. I basically wanted to get it working with the "minimal" number of changes first, then we can do this type of stuff. So, we can add it to the MR or just do it later in a subsequent one.
Yes, we can just fix up the most obvious things that were added in this PR. If things were not ideal already in the original code, I think it's better to fix it up in subsequent PRs.
What is the logic behind wp
(working precision)? It means double precision, correct? It won't work with single precision, right?
What is the logic behind
wp
(working precision)? It means double precision, correct? It won't work with single precision, right?
See #57
Merging #40 (c4ddcc9) into main (96c279d) will increase coverage by
0.66%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #40 +/- ##
==========================================
+ Coverage 88.11% 88.77% +0.66%
==========================================
Files 2 2
Lines 1220 1221 +1
Branches 459 456 -3
==========================================
+ Hits 1075 1084 +9
Misses 40 40
+ Partials 105 97 -8
Impacted Files | Coverage Δ | |
---|---|---|
src/minpack_capi.f90 | 98.38% <ø> (ø) |
|
src/minpack.f90 | 88.26% <100.00%> (+0.70%) |
:arrow_up: |
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 96c279d...c4ddcc9. Read the comment docs.
OK to merge? [Will address the code duplication issues in #61]
work in progress..
Closes #39 Closes #32 Closes #21 Closes #7 Closes #6