aburrell / apexpy

A Python wrapper for Apex coordinates
MIT License
34 stars 25 forks source link

[BUG]: Update apexshdat with IGRF13 #64

Closed asreimer closed 3 years ago

asreimer commented 3 years ago

This PR makes several changes that complete the update to IGRF13 coefficients:

  1. Propagate the usages of an external igrf13coeffs.txt to all fortran programs and subroutines that use the COFRM subroutine in magfld.f. This includes: apex.f, makeapexsh.f90, and checkapexsh.f90.
  2. Updates the docstrings in the updated fortran source files
  3. Updates the src/fortranapex/readme.txt to include some dev notes
  4. Updates the fortranapex.pyf file
  5. Update docs/maintenance.rst to include instructions for updating apexsh.dat.
  6. Updated the expected output for some unit tests because some outputs have changed by tiny amounts due to update to IGRF13 coefficients.

edit: Interesting note: according to the docstring of magfld.f in v1.0.2, is missing information about upgrading the file to IGRF12. If you compare the hard coded table of g(n,m) and h(n,m) in magfld.f with the IGRF12 coefficients file, they match up. The Emmert paper was published in 2010, so things were updated sometime between while building apexpy?

Comparing with this: https://github.com/NCAR/apex_fortran/blob/master/test.out, I get similar results. Note that we expect small errors comparing against the full apex code as stated in Emmert 2010.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 674


Totals Coverage Status
Change from base Build 650: 0.0%
Covered Lines: 375
Relevant Lines: 383

💛 - Coveralls
aburrell commented 3 years ago

The Emmert paper was published in 2010, so things were updated sometime between while building apexpy?

Yes, in a discussion with Emmert, he stated that Maute had done the more recent updates for IGRF.

asreimer commented 3 years ago

Thanks for suggesting those changes. I agree with and committed them.

Adding the tolerance to the unit tests is probably a good idea too. Although it's odd (concerning?) that we get different answers on macOS (or was it Windows?) vs. Fedora 33. Shouldn't we get the same numbers? AFAIK, Emmert's apex code is deterministic.

I wonder if adding something like: extra_f90_compile_args=['-fPIC'] to setup.py would fix this? -fno-automatic might also be needed. I had some issue like this in my flipchem package, where different OSes had different results due to different implicit compiler flags. Reading the Makefile, maybe we should at least include -fPIC as an extra compile arg? This should probably be addressed in a separate issue.

aburrell commented 3 years ago

I wonder if adding something like: extra_f90_compile_args=['-fPIC'] to setup.py would fix this? -fno-automatic might also be needed. I had some issue like this in my flipchem package, where different OSes had different results due to different implicit compiler flags. Reading the Makefile, maybe we should at least include -fPIC as an extra compile arg? This should probably be addressed in a separate issue.

I agree that we should look into this, it would be nice to ensure more consistent performance across OSes if there isn't a significant hint to performance. But having this as a separate issue would be best. That way we can test both the consistency and the runtime.