part of cam6_3_098: always pass NDEP from CAM and remove sst specs

[mvertens]

This PR addresses issue #104 and enables CAM to read in ndep (using the CDEPS inline API) from forcing files if it is not computing NDEP prognostically
As a result - CAM will ALWAYS send NDEP to the mediator
Right how, CLM is not using the passed NDEP from the streams - since it is still reading the drv_flds_in for NDEP
Once this PR is accepted the drv_flds_in entry for ndep can be removed and CLM can always accept NDEP from either CAM or DATM.
New XML variables are introduced to specify the stream forcing. This is being done in CTSM as well for all of its CDEPS stream specific variables.

I have run the following test suite on cheyenne with bfb results (other than the FIELDLIST is different for non-WACCM cases since NDEP is now being passed from CAM.
./create_test --xml-category aux_cam --xml-machine cheyenne --xml-compiler intel --compare cam6_3_088 --baseline-root /glade/p/cesmdata/cseg/cam_baselines/cesm_baselines

NOTE: that CTSM also needs a new PR in order to accept these new fields from CAM rather than use its own internal streams.

Replaces #729 (previous PR which replaced with this one)

Closes #104

[mvertens]

@cacraigucar - sorry for the confusion with this PR. This PR has been tested and should be the code to review. I am happy to answer any questions you have and even have a zoom chat if that would be helpful.

[cacraigucar]

A couple of questions to aid my review

[cacraigucar]

I'm being dense right now - what exactly is this checking? I see later that these variables are being set via the namelist, so wondering why the error message say they must be changed via xmlchange?

[mvertens]

These new namelist variables can ONLY be changed via xml variables - not in user_nl_cam settings. So this is checking that you did not try to change this variable in user_nl_cam and then issues a warning and aborts. Does that make sense?

[cacraigucar]

What is confusing me then, is why are they being listed in namelist_defaults.xml and namelist_definition.xml? As far as I was aware, all variables in the namelists were ones that could be controlled by user_nl_cam. We have configure options (like -phys cam6) which control the setup and are not changeable. These variables are feeling like they fit into the configure type of variables. Perhaps a configure option like "stream_ndep" and then it sets these 5 variables?

A second question about this is, why have these 5 variables exposed at the build-namelist option level? Up until now, there were only a few options and they were very high level like "ntasks".

@nusbaume, @jtruesdal, @fvitt and/or @peverwhee - Do any of you have more experience with build-namelist and have an opinion on this?

[gold2718]

why are they being listed in namelist_defaults.xml and namelist_definition.xml?

I may be missing something but I think this is a collision between the new (CIME) way of handling namelists and the old-style namelist_d*.xml files used by CAM. In a new namelist_definition_<comp>.xml file, this dependency is noted so that this sort of confusion does not exist. For instance, look at the definition of nx_global in <cam_root>/components/cdeps/datm/cime_config/namelist_definition_datm.xml.

We will (eventually) grow out of this with the new infrastructure.

[cacraigucar]

@gold2718 - Thank you for the clarification.

[nusbaume]

@mvertens @gold2718 just wanted to double-check if this XML-only requirement is specific to either CDEPS-managed input streams and/or variables that need to be coordinated with other components (e.g. CTSM)? If not, is there a good way to know when a variable should be managed by XML files rather than user_nl_cam?

[mvertens]

@nusbaume - if I am understanding your question correctly the idea of putting this in env_run.xml was to coordinate this with datm - so that both datm and cam could read the same xml variables for nitrogen deposition forcing. So since CAM is using streams via CDEPS shared code - it helps if the streams are defined the same way in both. Does that make sense?

[brian-eaton]

@mvertens, I understand that you want the new namelist vars, stream_ndep_, to only be settable via the corresponding case variables, CAM_STREAM_NDEP_. This is similar to the namelist variables that are set in config_component.xml in the CAM_NAMELIST_OPTS case variable. My suggestion is that rather than passing the variables as separate arguments to build-namelist, they could be added to the atmexp namelist group that's created in buildnml and passed to build-namelist using its -namelist argument. That would give these variables the same level of precedence that the CAM_NAMELIST_OPTS variables have, i.e., they can only be changed with xmlchange commands. Doing it this way would require no changes to the build-namelist script.

[brian-eaton]

Sorry about my formatting issue in the previous comment. Trying again... I meant stream_ndep_* and CAM_STREAM_NDEP_*. Need to remember to use preview:)

[brian-eaton]

@cacraigucar, @gold2718 - I thought it might be worth a quick note here addressing Cheryl's question about why the new namelist vars are in the namelist_defaults.xml and namelist_definition.xml files. Since these variables are being set by config_component.xml so the values can be shared by DATM and CAM, then assuming config_component.xml is taking responsibility to always set their values, the variables do not need to be set in namelist_defaults.xml. However, they still must be in the namelist_definition.xml file in order for build-namelist to put them in the atm_in file. The definition file contains metadata about the namelist group that the variables belong to. And in the case of varibles that provide filepaths, it also contains the metadata that tells build-namelist to put those filepaths in the cam.input_data_list file.

[mvertens]

@cacraigucar - I believe that I have addressed the changes you requested and that this PR is now ready for another review.

[fvitt]

If we are always passing 2 nitrogen deposition fields to the mediator via Faxa_ndep it seems we can get rid of Faxa_nhx and Faxa_noy. Furthermore, I don't see the need for ndep namelist settings in drv_fld_in. Using ndep_list setting in drv_flds_in is a bit of a convoluted way of determining if chemistry will provide the N dep fluxes or if they need to be read from file.

[fvitt]

How does this work for cases that have prognostic nitrogen deposition and do not need to read an ndep stream file?

[mvertens]

@fvitt - you still need the drv_flds_in to determine if waccm will be sending the data or not. The new logic is in atm_import_export.F90:

 if (ndep_nflds > 0) then
   ! The following is when WACCM computes ndep
   call set_active_Faxa_nhx(.true.)
   call set_active_Faxa_noy(.true.)
else
   ! The following is used for reading in stream data
   call set_active_Faxa_nhx(.false.)
   call set_active_Faxa_noy(.false.)
end if
! Assume that 2 fields are always sent as part of Faxa_ndep
call fldlist_add(fldsFrAtm_num, fldsFrAtm, 'Faxa_ndep', ungridded_lbound=1, ungridded_ubound=2)

I agree with you that this is not optimal and we should remove the need for drv_flds_in. However, CTSM is currently ignoring the NDEP sent to it from CAM in the case where ndep_nflds=0. There was a PR I did that removed the internal read of NDEP in CTSM - but unfortunately I did not restrict it to just NDEP and so it is currently on hold. My hope is that CAM can independently always send NDEP and that when the other components start using it and removing any internal read they have - then we can come up with a better mechanism that drv_flds_in for NDEP. Does that make sense?

[fvitt]

Is datm also sending the prescribed N dep fluxes via Faxa_ndep? Looking at the CTSM code, it seems that clm needs ndep_flds>0 to receive the fluxes in Faxa_ndep. Your logic is confusing to me. Your proposed changes are to have CAM always send N dep fluxes via Faxa_ndep and I believe datm will do the same. Thus, ndep_nflds will always need to be > 0.

There are more straightforward ways to determine if chemistry can provide the N dep fluxes.

BTW, WACCM is not the only configuration capable of providing prognosed N dep fluxes, low-top CAMChem can also provide them.

[mvertens]

@fvitt - yes datm is sending the prescribed Ndep fluxes to the mediator via Faxa_ndep. But it does not have to be that name for CAM. There are aliases that can be set up in the mediator in fd_cesm.yaml.
My method of determining if ndep should use streams was only an extension of what was there already. If there are more straightforward ways - please described them in an issue. The method I have implemented should be totally backwards compatible with what is already there. I think more straightforward implementations should be done in a separate PR at this point.

[fvitt]

Sorry if I am being dense.

In the clm code I see:

    ! from atm - nitrogen deposition
    call shr_ndep_readnl("drv_flds_in", ndep_nflds)
    if (ndep_nflds > 0) then
       call fldlist_add(fldsToLnd_num, fldsToLnd, Faxa_ndep, ungridded_lbound=1, ungridded_ubound=ndep_nflds)
       ! This sets a variable in clm_varctl
       ndep_from_cpl = .true.
    end if

So clm is going to receive the Faxa_ndep flds only if ndep_nflds>0. Correct?

Meanwhile you are assuming ndep_nflds>0 when the N dep fluxes are prognosed and do not need to be read from the stream input files. And the N dep fluxes from CAM will always be sent to the mediator via Faxa_ndep, regardless if they are from the stream inputs or prognosed.

Are there some proposed changes to CLM that I am missing? It seems that CLM will need to always look for the N dep fluxes in Faxa_ndep independent of the ndep_nflds setting. Otherwise, CLM will receive the N dep fluxes only if they are prognosed in CAM.

The Faxa_noy and Faxa_nhx fields should be removed from the system. They just add to the confusion in my opinion. I agree things can be cleaned up in another PR which no one will have time to work on...

[mvertens]

@billsacks -

• CAM still sets drv_flds_in for NDEP - since it uses that information to determine if CAM itself needs to read in NDEP from a stream file or use a prognostic calculation. That has not changed. @fvitt pointed out that there are better ways to do this in CAM and this has been deferred to a future PR.
• After this PR is merged it will be up to CTSM to remove the CTSM logic. I did that already in the PR that needs to be reimplemented at this point. CTSM should never need to look into drv_flds_in again for getting NDEP - since it will always be passed in.
• The key point that Francis noted that is the sum of Faxa_noy and Faxa_nhx is all that needs to be passed and not each individual field. All components just sum things up after they get the fields - so sending each field separately is not needed. If CAM and DATM only send the sum then this needs to be a change that is coordinated with multiple components.
  Does this help help clarify your questions?

[mvertens]

@fvitt - in regards to your question as to what testing has been done for the configurations where CAM is reading and sending the prescribed N dep stream datasets - I did careful testing and examined the differences in CTSM history files between the internal read of NDEP in CTSM and what it got when the data came from CAM. Differences were only seen after the 8th place. This should probably be confirmed again.

[billsacks]

@mvertens - thanks. Your responses help, but to make sure I understand this correctly: Is it correct that, even after this PR, in the case where CAM is reading NDEP from a stream, NDEP will not be present in the drv_flds_in file?

[mvertens]

@billsacks - sorry for not being clear. There CAM does not create an ndep namelist entry if it needs to read ndep from streams. This is exactly the same behavior as before. So the call to shr_ndep_readnl will return a value of ndep_nflds of 0 - from both CAM and CTSM. When CAM sees the ndep_nflds as 0 - it will read it in from a stream. When CTSM currently sees the ndep_nflds as 0 it will also read it from a stream otherwise if ndep_nflds > 0 it will use the data sent from the mediator (which is the data that CAM sends the mediator). This can clearly all be simplified once CTSM can always just use the data from CAM. Is this clear now?

[billsacks]

@mvertens - yes, that helps clarify things (and is consistent with what I thought but wasn't sure about). Thank you!

[nusbaume]

Looks good to me, assuming Cheryl and Francis's concerns are resolved. Otherwise I mostly just have some small typo fixes and a few questions.

[nusbaume]

@mvertens @gold2718 just wanted to double-check if this XML-only requirement is specific to either CDEPS-managed input streams and/or variables that need to be coordinated with other components (e.g. CTSM)? If not, is there a good way to know when a variable should be managed by XML files rather than user_nl_cam?

[mvertens]

@nusbaume @cacraigucar @fvitt - I have addressed (hopefully) the issues that were brought up. I am happy to iterate on this in case I missed something. Sorry for the confusion regarding the namelist_definition.xml file. I have removed the relevant section with ndep settings that is no longer needed.

[nusbaume]

@mvertens Thanks for resolving our concerns, and we totally understand the delay, given what we hear from Steve about how crazy your lives are right now.

Anyways, it does look like there are some changes that appear to be missing, at least from what I can see on Github? Please let us know (whenever you get the chance) if there was just a forgotten push or two, or if everything looks good on your end. Thanks!

[cacraigucar]

Thanks @mvertens. All of my concerns have been addressed. I bet y'all are glad you are settling into a more permanent location!

[mvertens]

@cacraigucar - thanks! And yes - we are very happy right now.

[nusbaume]

Everything looks great now, thanks @mvertens! I found one minor text change request, but otherwise this PR is good to go from my end.

[nusbaume]

Grammar change here (of for -> for).

[mvertens]

Thanks for catching this. I've put in the fix.

[gold2718]

Looks good to me!

[gold2718]

@billsacks, you are listed as a reviewer. Can you please either approve it, ask for changes, or remove yourself as a reviewer?

[billsacks]

I think I am only listed as a reviewer because I left review comments. I can't see how to remove myself from the list given that I have already left comments. I have no need to review this. I could mark it as approved if that would help your process, though I'm hesitant about setting the precedent of approving something that I haven't really reviewed at all.

[cacraigucar]

@billsacks - I am fine leaving your reviewer status as it is. It indicates to the right that you left comments and I agree that I would not want you to just check the approve review box without actually reviewing it. I too was unable to remove you from the reviewer list. I do review folks comments to see whether I should ask for a review or not from them, and I had decided I didn't need to get one from you. @brian-eaton should be moving this PR along shortly.