marbl-ecosys / MARBL

Marine Biogeochemistry Library
https://marbl-ecosys.github.io
Other
13 stars 21 forks source link

Support mks unit system #430

Closed mnlevy1981 closed 10 months ago

mnlevy1981 commented 1 year ago

This PR will handle the cgs -> mks conversion in its entirety, rather than focusing on diagnostics first and parameters later. The approach we have decided on:

  1. introduce optional unit_system argument to marbl_instance%init()

    1. If present, call subroutine in marbl_settings_mod.F90 to update several module variables (unit_system is a string that stores the desired unit system, cm2len converts from cm to proper length unit, and g2mass converts from grams to proper mass unit); defaults are 'cgs', 1._r8, and 1._r8.
    2. When setting default parameter values, cm2len and g2mass will be used to convert from cgs to selected system (e.g. caco3_bury_thres_depth = caco3_bury_thres_depth * cm2len)
  2. Update python script that generates settings file to allow --unit-system argument (so POP will generate marbl_in using cgs while MOM generates marbl_in in mks)

  3. Update diagnostic metadata so units are in proper system

  4. There are several fortran parameters (and some hard-coded "magic numbers" as well) that have units that will change based on the unit system

Fixes #41

mnlevy1981 commented 11 months ago

@klindsay28 and I have had a couple of conversations about determining appropriate thresholds for netcdf_comparison.py and making sure differences between cgs and mks are really the propogation of round-off level errors due to unit conversion. After merging #436 I compared gnu on derecho with gnu on my Mac (both of these are cgs, my mac has an intel chip):

$ ./netcdf_comparison.py -b ../tests/input_files/baselines/call_compute_subroutines.history.nc -n ../tests/regression_tests/call_compute_subroutines/history_1inst.nc --strict loose
Comparing ../tests/regression_tests/call_compute_subroutines/history_1inst.nc to the baseline ../tests/input_files/baselines/call_compute_subroutines.history.nc

Variable: Fe_scavenge
... Max relative error (3.6994203973617207e-11) exceeds 2e-11

Variable: Fefree
... Max relative error (4.3527441876818616e-11) exceeds 2e-11

Variable: J_Fe
... Max relative error (6.007586938336526e-11) exceeds 2e-11

Variable: Lig_scavenge
... Max relative error (3.699352433955277e-11) exceeds 2e-11

Variable: P_iron_PROD
... Max relative error (3.6994203973617207e-11) exceeds 2e-11

Differences found between files!

So I think the threshold change in 53a5632 is justified (and should be increased further, to 1e-10, so derecho vs baseline passes).

Although it's worth noting:

  1. The threshold was previously raised due to some remin terms failing, and those are not the same variables that are getting flagged in the hardware comparison.
  2. cgs vs mks passes on derecho, even with the old threshold

The important takeaway is that I'm pretty convinced the unit conversion has been done correctly, and it would be nice to modify the test thresholds to reflect that (I want a threshold where derecho vs baseline passes and cgs vs mks passes)