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

Update ecosys forcing #33

Merged
merged 5 commits into from
Jun 3, 2020
Merged

Conversation

mnlevy1981
Copy link
Collaborator

Description of changes:

Added two extra namelist variables to ecosys_forcing_mod.F90 and gave two other existing variables clearer names to improve iron flux computation.

Testing:

Test case/suite: aux_pop and aux_pop_MARBL (also built / ran a case on hobart_nag)
Test status: Definitely answer changing -- I presume not climate-changing

Fixes #17

User interface (namelist or namelist defaults) changes? iron_frac_in_atm_bc and iron_frac_in_seaice_bc are no longer in the namelist, while atm_bc_fe_bioavail_frac, atm_fe_to_bc_ratio, seaice_bc_fe_bioavail_frac, and seaice_fe_to_bc_ratio are

Turned in to atm_bc_fe_bioavail_frac  & atm_fe_to_bc_ratio
Turned in to seaice_bc_fe_bioavail_frac & seaice_fe_to_bc_ratio
@mnlevy1981
Copy link
Collaborator Author

This is a draft pull request so we don't inadvertently merge it before addressing #17 (comment)

Also, I'd appreciate feedback on the descriptions I added to the new variables in namelist_definition_pop.xml.

dust_ratio_thres has a new default value, and the old one is saved for when
OCN_BGC_OPTION=cesm2.1; fe_bioavail_frac_offset default did not change, but I
added the cesm2.1 default for consistency with the cesm2.0 value.
@mnlevy1981
Copy link
Collaborator Author

I think this compset is ready for review. I've run the full test suites mentioned in the initial post with b76082b and with the ecosys tracer module enabledNLCOMP tests fail with

Differences in namelist 'ecosys_forcing_data_nml':
  missing variable: 'iron_frac_in_atm_bc'
  missing variable: 'iron_frac_in_seaice_bc'
  found extra variable: 'atm_bc_fe_bioavail_frac'
  found extra variable: 'atm_fe_to_bc_ratio'
  found extra variable: 'seaice_bc_fe_bioavail_frac'
  found extra variable: 'seaice_fe_to_bc_ratio'

BASELINE tests fail for almost all the same compsets, though I was surprised to see that C1850ECO_ECOCESM20 compset tests were bit-for-bit identical; I would expect the fortran changes to change answers, so perhaps the test (or compset) is not doing what we expect? Note that those compsets see the same NLCOMP failure as the rest of the BGC compsets...

@mnlevy1981 mnlevy1981 marked this pull request as ready for review May 19, 2020 13:28
@mnlevy1981 mnlevy1981 requested a review from klindsay28 May 19, 2020 13:28
@mnlevy1981
Copy link
Collaborator Author

I have a few questions that may result in more changes to the code; some of these have been asked elsewhere but for completeness I'm compiling them all into a single post:

  1. (from Update ecosys forcing #33 (comment)) Are the descriptions in namelist_definitions_pop.xml for the four new namelist variables okay
  2. Should we be worried about the CESM 2.0 settings producing bit-for-bit results? (I'll investigate this after submitting this comment)
  3. Should there be a C1850ECO_ECOCESM21 (or C1850ECO_ECOCESM2OMIP) compset analogous to the C1850ECO_ECOCESM20 but for CESM 2.1 settings?
  4. (from @matt-long's bug in iron flux computation #17 (comment)): should the new partially-coupled default values go in via this PR or do we need to do some runs with this branch before computing the correct values?

@mnlevy1981
Copy link
Collaborator Author

To answer (2), it makes sense that the CESM2.0 test is bit-for-bit. The code we modified is only used when iron_flux_source = 'driver-derived' and the default for the CESM2.0 compset is monthly-calendar:

<iron_flux_source>driver-derived</iron_flux_source>
<iron_flux_source ocn_coupling="partial" ocn_bgc_config="cesm2.0">monthly-calendar</iron_flux_source>

phew :)

@mnlevy1981
Copy link
Collaborator Author

Testing on 6a7cef2 is complete, and the summary from #33 (comment) still holds (expected BASELINE and NLCOMP failures, no unexpected FAILs)

@mnlevy1981
Copy link
Collaborator Author

After review with @klindsay28, we decided not to support the old cesm2.0 and cesm2.1 parameter values for dust_ratio_thres, fe_bioavail_frac_offset, and dust_ratio_to_fe_bioavail_frac_r since the parameterization formula itself has changed (and we decided it wasn't worth the additional maintenance to keep the buggy parameterization available as an option -- users who want the CESM 2.1 parameterization should run CESM 2.1). I also need to clean up the long name descriptions of atm_bc_fe_bioavail_frac and seaice_fe_to_bc_ratio in the xml file, and will open a new issue ticket to address the inconsistency in variable names relating to iron (we have a mix of iron and fe in the variable name)

Got rid of "cesm2.1" and "cesm2.0" defaults for some parameters, edited
description of new namelist variables in the definition file
@mnlevy1981 mnlevy1981 merged commit 09112aa into ESCOMP:master Jun 3, 2020
@mnlevy1981 mnlevy1981 deleted the update_ecosys_forcing branch March 30, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug in iron flux computation
1 participant