Fe updates from UCI #464
Open
Fe updates from UCI #464
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cleaned up some trailing white space, converted hard tabs to soft, and added a few missing _r8 modifiers
Stand-alone build found some unused variables and imports that I removed
Also removed trailing whitespace I inadvertantly added to marbl_diagnostics_mod in the last commit
Some settings in marbl_settings_mod.F90 only need to be defined if lvariable_PtoC (or NtoC) is true. Also, found a couple block in marbl_init_tracer_metadata_mod.F90 that wasn't verifying N_ind > 0 before modifying N tracers (N_ind = 0 if lvariable_NtoC = .false.)
For cesm2.0 and cesm2.1 (3p1z or 4p1z), add lvariable_NtoC but set the default to .false.; also renamed gQfe_0 -> gQfe_max and added the non-N settings from the settings_latest files.
Don't know what to use for Qn_fixed, waiting to hear back from UCI and NOAA folks... so this will still fail the init.py test (but with fewer MARBL ERROR lines)
Also generate the 4p2z settings file from JSON as part of testing
After discussion with Keith L, it looks like Qn_fixed and Qn_zoo were added to the code base so the fixed N:C configuration looked more like the fixed P:C configuration... but it isn't necessary since we want to use the same fixed N:C throughout the code (as opposed to fixed P:C, which really does vary from among the different functional types even when fixed spatially / temporally)
Modify marbl_io_mod to initialize N tracers to C tracers * 16/112 and set feredsedflux and feventflux = 0. Modify marbl_diagnostics_mod.F90 to include dust%remin*dust_to_Fe in integrand, not righthand-side of iron conservation check Modify marbl_interior_tendency_mod.F90 to sflux_in * desportion in P_iron%remin before computing sflux_out. Also cleaned up a few small things in interior_tendency_mod (use unit_system instead of assuming cgs; initialize Lig_prod and declare it as inout in some routines)
no coccoC in this initial condition file, but coccoC = 0.07*spC
1. Had some hard-coded units instead of using unit_system for new variables 2. Want marbl_status_log to be last argument to subroutines whenever possible
PON%remin is PON%sflux_in * dzr_loc + ..., not PON%sflux_in + dzr_loc + ...
CI was failing the metadata check tests; now those tests should pass
If running fixed N:C, autotrophs don't have N tracer
This was referenced Feb 21, 2025
Some new variables added to diagnostics_latest.yaml are only available in runs where the base bio tracers are active.
There was a discrepancy in the units listed in the YAML file and the Fortran for SiOpt, FeOpt, POPt, and NOpt, and it turned out the units were incorrect in both places.
Four of the Redfield ratios are now user-configurable, and two of those four will have different default values when running with variable N:C instead of fixed N:C. The other 8 ratios depend of the values of the user-configurable ratios, and 1 of these 8 has a different formula when running with variable N:C instead of fixed.
Previous commit added new variables to settings_latest, this updates the rest of the files.
Block defining ABIO tracers appeared twice somehow...
1. Make sure intent(in) arguments precede intent(inout) or intent(out) in all subroutines 2. Remove NEW CODE blocks from diagnostics_mod (where failing conservation tests print out each individual term in the sum); while additional output would be useful, we want process-based terms, not individual terms 3. Remove new lines that contain commented out variable definitions and usage 4. Revert some white-space changes surrounding do-loops, if-statements, and comments
Trying to keep blocks within an associate statement declaration aligned
Just a small typo in variable description
Aligning # in multi-line comments
These two files are created with
./MARBL_generate_settings_file.py -f ../defaults/json/settings_latest+4p2z.json
-i ../tests/input_files/settings/marbl_with_o2_consumption_scalef.settings
and
./MARBL_generate_settings_file.py -f ../defaults/json/settings_latest+4p2z.json
-i ../tests/input_files/settings/marbl_with_o2_consumption_scalef.settings
-u mks
1. Previous commit swapped order of store_diagnostics_phosphorus_fluxes() and
store_diagnostics_phosphorus_fluxes(); this commit restores order used on
development branch
2. Better comment describing rmean_CaCO3_bury_integral
3. Consistent use of > instead of .gt. in marbl_init_tracer_metadata_mod.F90
4. Remove PON_bury_coeff from particulate_share_type
5. Only pass k level of Fefree, Fe_scavenge_rate, Fe_scavenge, and Lig_scavenge
to compute_scavenging(); only pass k level of Fe_scavenge to
compute_large_detritus_prod()
6. Remove comment about gQn being an output in some associate statements when
it is treated as an input (and one of those organizes by inputs and outputs
so I moved the "gQn =>" line up to the input section)
7. Indented an if-else block in compute_grazing()
8. Move definition of dzr_mod to right before it is first used, since that is
where the comment describing the variable is
9. Removed two newlines that were added inadvertently
10. Removed commented block of code from compute_denitrif()
11. gQfe_max, gQp_max, and gQn_max are maximums, not initial values. Updated
comments accordingly (only in src/, still incorrect in the settings YAML /
JSON files -- those will get updated once we have a test to ensure
consistency between YAML and Fortran)
|
ff9a042 and d335418 addressed the majority of comments from @klindsay28 during our most recent code review. Still outstanding:
|
Grabbed updated iron sediment flux and new red sed / vent fluxes from POP forcing file; also updated test suite to read the new fluxes from file
Updated marbl_init_drv.F90 to add ability to write a basic YAML file with the schema (shortname): longname: (longname) units: (unit) For every setting registered in marbl_settings_type. The new settings_metadata_check.py script reads in a JSON settings file and also the YAML file written by init.py and flags notes any of the following: 1. Variables where the longname does not match 2. Variables where the units do not match 3. Variables that are in the fortran output but not the JSON file 4. Variables that are in the JSON but not in the fortran output run_test_suite.sh already generates settings files from every JSON file and runs init.py; now it runs settings_metadata_check.py afterwards. It also includes that python script when running pylint. (pylintrc was updated so it does not report duplicate code between the two metadata_check.py scripts). netcdf_metadata_check.py was updated because there was an indentation error at the very bottom of the file.
Some of the metadata in marbl_settings_mod.F90 was wrong, and some of the changes were in the YAML / json file. I also updated the testing script itself to account for the fact that particulate_flux_ref_depth is always provided in m (even when running in cgs, we use it to create diags like VAR_100m and don't want VAR_10000cm) Also want a .gitignore file in the init test dir to ignore settings_metadata.yaml
Both settings_latest and settings_latest+cocco now pass the metadata consistency check (match what is in the Fortran)
Settings for cesm2.0, cesm2.1, and cesm2.1+cocco all pass metadata consistency check (match what is in the Fortran)
This has been addressed:
This was done in 810e465
I have a response to my email, but still need to apply the recommended fixes. Once those code mods are in and reviewed, I'll update the baselines for our test suite so we pass all the CI tests |
PON_bury_coeff is a scale-factor that should be applied to POC_bury_coeff rather than treated as the actual PON burial coefficient. It was being used as the latter in the PON%sed_loss computation, and now is properly being used as the former
The variable N:C and new iron forcings both changed answers. Baselines now include those modifications
Real variables have units of '1'; logicals, strings, and integers that are
tracking non-physical quantities (like index of autotroph in grazing type) have
units of ''.
The init test now writes out units : '{units}', so empty strings show up as ''
and units of 1 are treated as a string instead of integer in
settings_metadata_check.py. (Also that python file did not set error_found if
the only differences between the JSON and Fortran metadata was in the units;
that bug has been fixed).
|
As of the three most recent commits, this code appears to be ready to merge. I do want to run a few CESM / stand-alone MOM6 tests, but 🤞 there are no more changes to this PR before it gets merged. |
If init.py fails with a specific settings file, then we shouldn't compare the settings metadata afterwards
This subroutine calls setup_local_tracers() [which is now the public routine marbl_interior_tendency_setup_local_tracers()], which in turn calls autotroph_zero_consistency_enforce() to ensure tracer values for each autotroph are non-negative and, if any autotroph's tracer value is zero at a location then they all are. The new subroutine then updates this%tracer with the consistent autotroph values, leaving non-autotroph tracers unchanged.
When I first created a branch with the iron forcing updates, the call_compute_subroutines stand-alone test did not run; one of the issues was that Lig_prod(k) was not defined for k > kmt. I thought I added a line of code to set Lig_prod(k)=0 in that case, but inadvertantly was setting Lig_prod(k)=0 everywhere (overwriting the value computed earlier in the routine when k<=kmt). The fix is to move the Lig_prod(k)=0 line into the else block of the "if (k <= column_kmt)" statement
CI tests were failing with Could not import extension sphinx.builders.latex (exception: No module named 'docutils.utils.roman') According to https://docutils.sourceforge.io/RELEASE-NOTES.html the docutils/utils/roman.py file was removed in 0.22, so we require something older
Rather than hard-coding values for scaling fesedflux and feventflux when computing dust production, users can now control these via marbl_in
|
5745022 failed baseline comparison tests because the new |
dust_per_unit_fesedflux and dust_per_unit_feventflux are listed in all settings files; for settings_latest* the default values will match what is in the Fortran, while they are both set to 0 for cesm2.0 and cesm2.1 settings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch contains the code changes used by the UCI group; it includes variable C:N as well as new iron forcing