Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize the tests #40

Merged
merged 51 commits into from
Mar 13, 2022
Merged

Conversation

jacobwilliams
Copy link
Member

@jacobwilliams jacobwilliams commented Feb 18, 2022

work in progress..

Closes #39
Closes #32
Closes #21
Closes #7
Closes #6

@certik
Copy link
Member

certik commented Feb 18, 2022

Thanks for working on this!

@jacobwilliams
Copy link
Member Author

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).

@jacobwilliams
Copy link
Member Author

jacobwilliams commented Mar 6, 2022

Summary

test_hybrj

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

test_hybrd

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

test_lmder

All cases identical.

test_lmstr

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

test_lmdif

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

test_chkder

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

@jacobwilliams
Copy link
Member Author

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).

@jacobwilliams
Copy link
Member Author

Next step is adding the "truth" values to the tests so they are checked in the CI. Probably with some tolerance?

@jacobwilliams jacobwilliams changed the title WIP: modernize the tests Modernize the tests Mar 12, 2022
@jacobwilliams
Copy link
Member Author

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.

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/minpack.f90 Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
test/test_chkder.f90 Show resolved Hide resolved
scripts/validate.sh Outdated Show resolved Hide resolved
test/test_chkder.f90 Outdated Show resolved Hide resolved
@jacobwilliams
Copy link
Member Author

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.

@certik
Copy link
Member

certik commented Mar 12, 2022

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?

@jacobwilliams
Copy link
Member Author

What is the logic behind wp (working precision)? It means double precision, correct? It won't work with single precision, right?

See #57

@codecov
Copy link

codecov bot commented Mar 12, 2022

Codecov Report

Merging #40 (c4ddcc9) into main (96c279d) will increase coverage by 0.66%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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%) ⬆️

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.

@jacobwilliams
Copy link
Member Author

OK to merge? [Will address the code duplication issues in #61]

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me. Also fixes the runtime error with NAG.

@awvwgk awvwgk linked an issue Mar 12, 2022 that may be closed by this pull request
Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to merge.

@jacobwilliams jacobwilliams merged commit e26542b into fortran-lang:main Mar 13, 2022
@jacobwilliams jacobwilliams deleted the test-refactoring branch March 13, 2022 00:26
@awvwgk awvwgk added this to the v2.0.0 milestone Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants