marbl-ecosys / MARBL

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

Turned on check_r8_settings() test #347

Closed mnlevy1981 closed 4 years ago

mnlevy1981 commented 4 years ago

Fixed the errors flagged by the new check

I'll test this in CESM before opening it for review / merging

mnlevy1981 commented 4 years ago

Travis failed baseline comparison:

Comparing ../tests/regression_tests/call_compute_subroutines/history_1inst.nc to the baseline ../tests/input_files/baselines/call_compute_subroutines.history.nc

Variable: CaCO3_ALT_CO2_FLUX_IN
... Max relative error (3.936427517850028e-11) exceeds 1e-11

Variable: CaCO3_ALT_CO2_PROD
... Max relative error (2.8308536980301243e-09) exceeds 1e-11

Variable: CaCO3_ALT_CO2_REMIN
... Max relative error (3.9810329395373364e-11) exceeds 1e-11

Variable: CaCO3_FLUX_IN
... Max relative error (3.936427517850028e-11) exceeds 1e-11

Variable: CaCO3_PROD
... Max relative error (2.8308536980301243e-09) exceeds 1e-11

Variable: CaCO3_PROD_zint
... Max relative error (2.5051912195309552e-11) exceeds 1e-11

Variable: CaCO3_REMIN
... Max relative error (3.9810329395373364e-11) exceeds 1e-11

Variable: CaCO3_REMIN_zint
... Max relative error (1.8736397324465527e-11) exceeds 1e-11

Variable: CaCO3_form
... Max relative error (7.898906165705255e-11) exceeds 1e-11

Variable: CaCO3_form_zint
... Max relative error (4.0704915447119445e-11) exceeds 1e-11

Variable: CaCO3_form_zint_100m
... Max relative error (4.069403681413221e-11) exceeds 1e-11

Variable: DOC_prod
... Max relative error (2.0552164436216725e-10) exceeds 1e-11

Variable: DON_prod
... Max relative error (2.0552171049398593e-10) exceeds 1e-11

Variable: DOP_diat_uptake
... Max relative error (8.952403217606769e-11) exceeds 1e-11

Variable: DOP_diaz_uptake
... Max relative error (7.285889470261258e-11) exceeds 1e-11

Variable: DOP_prod
... Max relative error (1.6556298130850606e-10) exceeds 1e-11

Variable: DOP_sp_uptake
... Max relative error (7.898916339714164e-11) exceeds 1e-11

Variable: J_ALK
... Max relative error (2.9645487681580403e-10) exceeds 1e-11

Variable: J_ALK_ALT_CO2
... Max relative error (2.9645487681580403e-10) exceeds 1e-11

Variable: J_DIC
... Max relative error (7.165842107305618e-10) exceeds 1e-11

Variable: J_DIC_ALT_CO2
... Max relative error (7.165842107305618e-10) exceeds 1e-11

Variable: J_DOC
... Max relative error (9.130492402681196e-11) exceeds 1e-11

Variable: J_DON
... Max relative error (9.130488292012782e-11) exceeds 1e-11

Variable: J_DOP
... Max relative error (1.1734724840018293e-09) exceeds 1e-11

Variable: J_Fe
... Max relative error (1.8394487036843215e-09) exceeds 1e-11

Variable: J_Lig
... Max relative error (4.939580445114519e-10) exceeds 1e-11

Variable: J_NH4
... Max relative error (4.838547001107285e-10) exceeds 1e-11

Variable: J_NO3
... Max relative error (5.084992008377834e-09) exceeds 1e-11

Variable: J_O2
... Max relative error (4.5268263394583066e-08) exceeds 1e-11

Variable: J_PO4
... Max relative error (8.498111312543347e-10) exceeds 1e-11

Variable: J_SiO3
... Max relative error (8.00940047642529e-10) exceeds 1e-11

Variable: J_diatC
... Max relative error (5.535172817417481e-09) exceeds 1e-11

Variable: J_diatChl
... Max relative error (5.848566901295763e-07) exceeds 1e-11

Variable: J_diatFe
... Max relative error (3.054376994431516e-09) exceeds 1e-11

Variable: J_diatP
... Max relative error (5.535239371882309e-09) exceeds 1e-11

Variable: J_diatSi
... Max relative error (7.151137455311196e-10) exceeds 1e-11

Variable: J_diazC
... Max relative error (3.713727550625523e-10) exceeds 1e-11

Variable: J_diazChl
... Max relative error (5.114458003655235e-09) exceeds 1e-11

Variable: J_diazFe
... Max relative error (6.151572169622267e-10) exceeds 1e-11

Variable: J_diazP
... Max relative error (1.0675094767968518e-09) exceeds 1e-11

Variable: J_spC
... Max relative error (7.1836443183574174e-09) exceeds 1e-11

Variable: J_spCaCO3
... Max relative error (1.1705334843917489e-08) exceeds 1e-11

Variable: J_spChl
... Max relative error (3.085178188500079e-07) exceeds 1e-11

Variable: J_spFe
... Max relative error (1.969386389715136e-07) exceeds 1e-11

Variable: J_spP
... Max relative error (7.18375993519825e-09) exceeds 1e-11

Variable: Jint_Fetot
... Baseline is 0 at some indices where new data is non-zero

Variable: Jint_Ntot
... Baseline is 0 at some indices where new data is non-zero

Variable: Jint_Sitot
... Baseline is 0 at some indices where new data is non-zero

Variable: Lig_loss
... Max relative error (1.8631472817821692e-11) exceeds 1e-11

Variable: Nfix
... Max relative error (7.285863410229148e-11) exceeds 1e-11

Variable: O2_PRODUCTION
... Max relative error (8.794683772103037e-11) exceeds 1e-11

Variable: PO4_diat_uptake
... Max relative error (8.952405508513185e-11) exceeds 1e-11

Variable: PO4_diaz_uptake
... Max relative error (7.285897048897e-11) exceeds 1e-11

Variable: PO4_sp_uptake
... Max relative error (7.898911150958648e-11) exceeds 1e-11

Variable: POC_PROD
... Max relative error (1.2072019854832627e-09) exceeds 1e-11

Variable: POP_PROD
... Max relative error (1.2112196218758161e-09) exceeds 1e-11

Variable: SiO2_PROD
... Max relative error (6.77133592800775e-11) exceeds 1e-11

Variable: bSi_form
... Max relative error (8.952398423955888e-11) exceeds 1e-11

Variable: calcToFloor
... Max relative error (3.899681379548288e-11) exceeds 1e-11

Variable: calcToSed
... Max relative error (3.899681379548288e-11) exceeds 1e-11

Variable: calcToSed_ALT_CO2
... Max relative error (3.899681379548288e-11) exceeds 1e-11

Variable: diat_Qp
... Max relative error (6.771323393173146e-11) exceeds 1e-11

Variable: diat_bSi_form
... Max relative error (8.952398423955888e-11) exceeds 1e-11

Variable: diat_light_lim_Cweight_avg_100m
... Max relative error (4.752975111611705e-11) exceeds 1e-11

Variable: diat_light_lim_surf
... Max relative error (1.882370832391e-11) exceeds 1e-11

Variable: diaz_Nfix
... Max relative error (7.285863410229148e-11) exceeds 1e-11

Variable: diaz_Qp
... Max relative error (5.737435122664265e-09) exceeds 1e-11

Variable: diaz_light_lim_Cweight_avg_100m
... Max relative error (3.3935725842690296e-11) exceeds 1e-11

Variable: diaz_light_lim_surf
... Max relative error (1.9112022128790238e-11) exceeds 1e-11

Variable: photoC_NO3_TOT
... Max relative error (8.794698850903627e-11) exceeds 1e-11

Variable: photoC_NO3_TOT_zint
... Max relative error (4.6118291308824354e-11) exceeds 1e-11

Variable: photoC_NO3_diat
... Max relative error (8.952410436181879e-11) exceeds 1e-11

Variable: photoC_NO3_diat_zint
... Max relative error (4.815614608335385e-11) exceeds 1e-11

Variable: photoC_NO3_diaz
... Max relative error (7.285876482648345e-11) exceeds 1e-11

Variable: photoC_NO3_diaz_zint
... Max relative error (3.955913788738377e-11) exceeds 1e-11

Variable: photoC_NO3_sp
... Max relative error (7.898907126026599e-11) exceeds 1e-11

Variable: photoC_NO3_sp_zint
... Max relative error (4.256284140908512e-11) exceeds 1e-11

Variable: photoC_TOT
... Max relative error (8.794696204558916e-11) exceeds 1e-11

Variable: photoC_TOT_zint
... Max relative error (4.52425782885665e-11) exceeds 1e-11

Variable: photoC_TOT_zint_100m
... Max relative error (4.516740071402324e-11) exceeds 1e-11

Variable: photoC_diat
... Max relative error (8.952403761544082e-11) exceeds 1e-11

Variable: photoC_diat_zint
... Max relative error (4.762928336518365e-11) exceeds 1e-11

Variable: photoC_diat_zint_100m
... Max relative error (4.751965919310907e-11) exceeds 1e-11

Variable: photoC_diaz
... Max relative error (7.285887913738892e-11) exceeds 1e-11

Variable: photoC_diaz_zint
... Max relative error (3.4525154096617827e-11) exceeds 1e-11

Variable: photoC_diaz_zint_100m
... Max relative error (3.4142385930751094e-11) exceeds 1e-11

Variable: photoC_sp
... Max relative error (7.898909721158455e-11) exceeds 1e-11

Variable: photoC_sp_zint
... Max relative error (4.1975679584299033e-11) exceeds 1e-11

Variable: photoC_sp_zint_100m
... Max relative error (4.1954795083740195e-11) exceeds 1e-11

Variable: photoFe_diat
... Max relative error (8.952414307452144e-11) exceeds 1e-11

Variable: photoFe_diaz
... Max relative error (7.285889791452965e-11) exceeds 1e-11

Variable: photoFe_sp
... Max relative error (7.898902719985829e-11) exceeds 1e-11

Variable: photoNH4_diat
... Max relative error (8.95240373322248e-11) exceeds 1e-11

Variable: photoNH4_diaz
... Max relative error (7.285889818024022e-11) exceeds 1e-11

Variable: photoNH4_sp
... Max relative error (7.898923729201148e-11) exceeds 1e-11

Variable: photoNO3_diat
... Max relative error (8.952419050630276e-11) exceeds 1e-11

Variable: photoNO3_diaz
... Max relative error (7.285882959397508e-11) exceeds 1e-11

Variable: photoNO3_sp
... Max relative error (7.89891547829584e-11) exceeds 1e-11

Variable: sp_CaCO3_form
... Max relative error (7.898906165705255e-11) exceeds 1e-11

Variable: sp_CaCO3_form_zint
... Max relative error (4.0704915447119445e-11) exceeds 1e-11

Variable: sp_CaCO3_form_zint_100m
... Max relative error (4.069403681413221e-11) exceeds 1e-11

Variable: sp_Qp
... Max relative error (4.953108746599223e-09) exceeds 1e-11

Variable: sp_light_lim_Cweight_avg_100m
... Max relative error (4.213233245457484e-11) exceeds 1e-11

Variable: sp_light_lim_surf
... Max relative error (2.1655235039303592e-11) exceeds 1e-11

Variable: sp_loss_doc
... Max relative error (8.517812303030783e-10) exceeds 1e-11

Variable: sp_loss_poc
... Max relative error (2.4075210451658584e-09) exceeds 1e-11

Variable: sp_loss_poc_zint
... Max relative error (2.8737947097382387e-11) exceeds 1e-11

Variable: zoo_loss_doc
... Max relative error (2.380676239671203e-10) exceeds 1e-11

Variable: zoo_loss_poc
... Max relative error (1.4808916176193722e-09) exceeds 1e-11

Differences found between files!

These differences are expected, and are why this issue hasn't been addressed previously

mnlevy1981 commented 4 years ago

For completeness, here is the output of code_consistency.py if we check for missing _r8 on the current development branch:

$ ./code_consistency.py
Check Fortran files for coding standard violations:
* Check for hard tabs: 0 error(s) found
* Check for trailing white space: 0 error(s) found
* Check length of lines: 0 error(s) found
* Check for case sensitive statements: 0 error(s) found
* Check for r8: 6 error(s) found
  ../src/marbl_interior_tendency_mod.F90:1450:             picpoc = max(0.,0.104 * temperature - 0.108)
  ../src/marbl_interior_tendency_mod.F90:1456:           picpoc = max(0.,-0.0136 * CO2(:) + picpoc(:) + 0.21)
  ../src/marbl_interior_tendency_mod.F90:1459:           picpoc = max(0.,-0.48 * Plim(auto_ind,:) + picpoc(:) + 0.48)
  ../src/marbl_constants_mod.F90:58:       K_Boltz   =   8.617330350e-5,    & ! Boltzmann constant (eV/K)
  ../src/marbl_constants_mod.F90:60:       epsC      =   1.00e-8,           & ! small C concentration (mmol C/m^3)
  ../src/marbl_constants_mod.F90:61:       epsTinv   =   3.17e-8,           & ! small inverse time scale (1/year) (1/sec)
Fortran errors found: 6

Check python files for coding standard violations:
* Check for hard tabs: 0 error(s) found
* Check for trailing white space: 0 error(s) found
Python errors found: 0

Total error count: 6

This PR addresses those 6 lines and also ensures that Travis will run this test

klindsay28 commented 4 years ago

Some of the replacements are of the form 0. to 0._r8 and 1. to 1._r8. Should these instead be replaced with c0 and c1 respectively? If yes, should we do that replacement throughout the code base (perhaps this gets beyond the scope of this PR).

I'm now on the fence myself about having c0, c1, ... in marbl_constants_mod.F90. I see a justification for constants like pi there, but I'm not entirely convinced of the benefit of having numeric constants in there that are exactly representable in floating point.

mnlevy1981 commented 4 years ago

Some of the replacements are of the form 0. to 0._r8 and 1. to 1._r8. Should these instead be replaced with c0 and c1 respectively? If yes, should we do that replacement throughout the code base (perhaps this gets beyond the scope of this PR).

I'm now on the fence myself about having c0, c1, ... in marbl_constants_mod.F90. I see a justification for constants like pi there, but I'm not entirely convinced of the benefit of having numeric constants in there that are exactly representable in floating point.

Are the options just 0._r8, 0.0_r8, and c0, or is it safe to assume that compilers will accurately convert integers so 0 is also equivalent? I think I'm leaning towards using 0.0_r8 everywhere (and removing c0, c1, etc from marbl_constants_mod).

In the 30 minutes I've been thinking about it, I've actually cycled through several different options as my favorite, but in searching to see how often we use 0._r8 or 0.0_r8 I found lines like

parm_scalelen_vals = (/     1.0_r8,     3.6_r8,     4.7_r8,      4.8_r8 /)

And I don't particularly like the aesthetic of any of the following:

parm_scalelen_vals = (/     1,     3.6_r8,     4.7_r8,      4.8_r8 /)
parm_scalelen_vals = (/     c1,     3.6_r8,     4.7_r8,      4.8_r8 /)
parm_scalelen_vals = (/     1._r8,     3.6_r8,     4.7_r8,      4.8_r8 /)

If we trust the compiler to handle integer -> double precision exactly, a hybrid approach where we use 0 for variable assignment but 0.0_r8 elsewhere might make sense? I don't see any issue replacing

Tref = 0.0_r8

with

Tref = 0

Regardless, for this PR I think I should add the 0 in the tenth place of the decimals:

$ git diff
diff --git a/src/marbl_interior_tendency_mod.F90 b/src/marbl_interior_tendency_mod.F90
index 30cdc79..dbc199c 100644
--- a/src/marbl_interior_tendency_mod.F90
+++ b/src/marbl_interior_tendency_mod.F90
@@ -1446,17 +1446,17 @@ contains
           !For details, see Krumhardt et al., 2019, JAMES & Krumhardt et al., 2017, Progress in Oceanography

           !temperature effect (linearly decreases calcification at temps < 11C)
-          where (temperature < 11._r8)
-            picpoc = max(0._r8,0.104_r8 * temperature - 0.108_r8)
+          where (temperature < 11.0_r8)
+            picpoc = max(0.0_r8,0.104_r8 * temperature - 0.108_r8)
           elsewhere
-            picpoc = 1._r8
+            picpoc = 1.0_r8
           end where

           !CO2 effect (CaCO3/organicC ratio ('picpoc') decreases as CO2 increases)
-          picpoc = max(0._r8,-0.0136_r8 * CO2(:) + picpoc(:) + 0.21_r8)
+          picpoc = max(0.0_r8,-0.0136_r8 * CO2(:) + picpoc(:) + 0.21_r8)

           !P-limitation effect (CaCO2/organicC ratio increases when P limitation term is low)
-          picpoc = max(0._r8,-0.48_r8 * Plim(auto_ind,:) + picpoc(:) + 0.48_r8)
+          picpoc = max(0.0_r8,-0.48_r8 * Plim(auto_ind,:) + picpoc(:) + 0.48_r8)

           !multiply cocco growth rate by picpoc to get CaCO3 formation
           CaCO3_form(auto_ind,:) = picpoc(:) * photoC(auto_ind,:)

and then we can open another issue ticket to discuss dropping the constants?

klindsay28 commented 4 years ago

I agree with not liking the aesthetics of the 3 alternatives to parm_scalelen_vals.

I'm not in favor of the hybrid approach. I'd rather us just use 0.0_r8 everywhere. Having a single rule of thumb seems easier to me.

I got worried when you wrote that you want a 0 in the tenth place, until I realized that you want a zero in the tenths place. :)

Dropping the constants in another PR not only makes sense to me, but seems preferable. I think that that change should be bit for bit. Having it in a separate PR would ease demonstrating that.

mnlevy1981 commented 4 years ago

Okay, I think f0e82b8 gets us the consistency we want in this PR and I created #350 will to address the marbl_constants_mod.F90 clean-up.

I got worried when you wrote that you want a 0 in the tenth place, until I realized that you want a zero in the tenths place. :)

Argh, I was so focused on not writing "tens place" that I missed that particular typo on proofread

mnlevy1981 commented 4 years ago

@klindsay28 Does this look okay now? Every line that I touched uses .0_r8 for integer values cast as r8, which was mostly changing from 1._r8 -> 1.0_r8 but for epsC I switched from 1.00e-8_r8 to 1.0e-8_r8. I'm now noticing that K_Boltz = 8.617330350e-5_r8: my initial thought was "that trailing 0 provides some information about the precision to which we trust the value", but when I went digging more I learned that the accepted value of the Boltzmann constant was updated in 2017; Table 3 of that paper says

k = 1.380649e−23 ! J/K

which, per wikipedia, would be

k = 8.617333262145e−5 ! eV/K

I'll open another issue ticket for discussing whether we want to update this value (and what to update to)... my main point is that I don't feel like we need to drop the trailing 0 in the K_Boltz decimal but I'm happy to if others disagree.