HI @jupflug

Thanks for these fixes.

There are a few changes to the Noah-MP-4.0.1 code that would also affect users running without SnowModel.

We try to avoid making these types of changes unless absolutely necessary.

1. Why are there changes to NoahMP401_readcrd.F90 on lines 427, 445, and 566 that take out the check to see if the run is a "restart" or a "coldstart"?
2. In module_sf_noahmp_glacier_401.F90, on line 1211, the variable FV should be defined as "INOUT". This was a bug that was fixed a few months ago: 9b06a00
3. In module_sf_noahmplsm_401.F90, on line 4, why did you add LIS_coreMod to this routine? I don't think it's needed.
4. On line 692 of this same file, there's a formatting change that should be removed.
5. Same file, lines 745 to 752 are new lines that are not specific to SnowModel. Can they be wrapped in a IF(OPT_SNF == 5) THEN block?
6. Same file, lines 1127 to 1132 are another change that would affect folks not running SnowModel. These also need to be corrected.
7. Same file, line 1853 changed the threshold for SNOWH. I see why (to prevent dividing by a very small number on the next line). However, what are the implications of leaving FMELT and FSNO undefined if the code doesn't go into this IF block. Further, the FMELT "divide by a really small number check" was removed. Why?
8. Same file, lines 6841 and 6843, why was the maximum SWE changed from 2000mm to 10000mm?

As these changes affect multiple Noah-MP-4.0.1 codes and users when not running SnowModel, we need to resolve these before this pull request is merged.

Thanks @dmocko

A majority of these changes you note above were made to the code prior to my changes for the snow updates and precipitation partitioning. I'm looping in @karsenau, who can help us determine what to keep/revert.

Re. number 8 above, we commonly increase the maximum SWE since our simulations can often exceed 2m SWE, and capping it can result in unrealistic SWE evolution in these locations/years. This doesn't have to change for this pull request.

Hi @jupflug and @dmocko:

Thanks for the excellent questions and review, @dmocko. I would like to offer some insights and a couple of suggestions on how we may proceed with some of the code commits in this PR and for some future PRs.

Main Initial inputs: The original branch that this code was committed from is an older version and has had a few specific tweaks for specialized experiments. I would recommend to @jupflug to create a new separate branch from the latest "master" branch and make individual commits + PRs vs. one large set of them (as here), given the nature of the original branch itself and its purpose.

@sujayvkumar would like the updates that have been made to the "da_snow/noahmp401_snow_update.F90" routine incorporated sooner than the others, so we may want to follow my first suggestion (with implementation into the latest master branch) and test + provide on a test case that includes a snow DA update short run for that PR.

1. Why are there changes to NoahMP401_readcrd.F90 on lines 427, 445, and 566 that take out the check to see if the run is a "restart" or a "coldstart"?

• Some of these changes may have been related to specifc experiment needs and not sure why that is in there for the commit. I would not commit these changes.

2). In module_sf_noahmp_glacier_401.F90, on line 1211, the variable FV should be defined as "INOUT". This was a bug that was fixed a few months ago: 9b06a00

• Again, this was from an older branch and not up-to-date with master.

3). In module_sf_noahmplsm_401.F90, on line 4, why did you add LIS_coreMod to this routine? I don't think it's needed.

• I don't think we should include a call to LIS_coreMod either here, but it looks to be related to the LIS_localPet that shows up on line 2156.

5). Same file, lines 745 to 752 are new lines that are not specific to SnowModel. Can they be wrapped in a IF(OPT_SNF == 5) THEN block?

• Lines 745-750 along with line 1853 (re: 7. below) do relate to coupling of SnowModel, as NoahMP401 fails to run in coupled mode if these two sets of code are not in place. I did not commit these two updates to NoahMP401 when I committed the SnowModel code, as I wanted to make it a separate commit+PR as one of the code changes does might slight changes to NoahMP401. These updates would be recommended in the future, even if they seem minor. Otherwise, users will not be able to couple NoahMP401 and SnowModel. Further evaluations of these two additions made (look for "KRA") actually make sense, given any machine precision that has issues with very small numbers -- we have to ensure 0 for SNOWH and SNEQV when values are so very tiny for the call to the energy routine, I found. And for the second check, very small values near 0 (< 0.001) created issues if it entered this condition statement for density calculation. You don't want any division by "0", which occurred on a rare occasion with very small SNOWH values. I want to recommend these two updates in a future commit as a fix as well.

• Your suggestion of wrapping these two commits in with the new precipitation partitioning option (OPT_SNF==5) could be considered but feel free to consider my above experience of what I ran into, even from a non-SnowModel coupling runtime mode.

6). Same file, lines 1127 to 1132 are another change that would affect folks not running SnowModel. These also need to be corrected.

• I believe this was addition made by @mlwrzesien for experiment purposes, so this should be considered for separate commit + PR later too.

7). Same file, line 1853 changed the threshold for SNOWH. I see why (to prevent dividing by a very small number on the next line). However, what are the implications of leaving FMELT and FSNO undefined if the code doesn't go into this IF block. Further, the FMELT "divide by a really small number check" was removed. Why?

• Per your first question here, please see my response to 5) above. I think the bigger issue is to avoid any division by 0. If SNOWH is barely present than SWE will probably have even smaller values.
• Per your second question and observation, I believe that may be due to @bailinglee adding that code later in LIS master that is not reflected in this older branch that was submitted as part of this PR. We would want to keep that check in place.

8). Same file, lines 6841 and 6843, why was the maximum SWE changed from 2000mm to 10000mm?

• This has been an issue that has plagued the Snow community for a long time. Some of the NoahMP physics was taken from CLM code, where an upper limit was imposed on SWE so "run-away" glaciers could not form in the model. I don't know what the NoahMP community has planned for the current and upcoming releases, but it would be good to make this configurable, as for the climate modeling communities who run at coarser scales (e.g., 10 km and >) and the snow hydrology community who want to run models at < 1 km, this upper limit has been an issue. Could we make this a configurable setting now in our version of LIS - NoahMP401? It would be helpful for these different applications.

Thank you and please let me know if you would like to discuss further offline next week.

Thanks @karsenau! I agree that I tried to push too much. This can all be easily separated into multiple PRs that should be much easier to follow.

I updated the name of this PR, and I'll revert to commit only those changes to noahmp401_snow_update.F90. I can follow later with the additional precip partitioning scheme and we can chat as a group about how/when we want to work on committing the rest of the changes.

@karsenau @dmocko and @sujayvkumar, please let me know if you have any issues with that plan.

Hi @jupflug and @karsenau

Thanks for the updated commits and for only including the Noah-MP-4.0.1 snow DA update fixes in this PR.

Thanks also for your detailed responses to my questions.

Yes, it's better to have PRs only change a single topic instead of combining many of them into a single PR.

I would be glad to meet with you both next week to discuss the plan for the precipitation partitioning and other changes.

I am taking you at your word that @sujayvkumar approves of these changes to the Noah-MP-4.0.1 snow DA updates.

1. Would you mind changing the name of this PR again? Something more specific, such as "Noah-MP-4.0.1 snow DA updates", for example.
2. It's better for you to create a branch with your changes for a PR instead of trying to merge in your master branch. Follow the LISF Working with GitHub guide, and look at Sections 4.7.7 and 4.7.11 here: https://nasa-lis.github.io/LISF/working_with_github/working_with_github.html
3. This PR is on the line between a "bug" and an enhancement. But it seems to be more of a "bug", so probably Section 4.7.11 is better to follow, and this PR should be merged into the support/lisf-public-7.4 branch instead of master. But if you want it merged into master for other development, I would be OK with it.

I wasn't able to change the head of this PR, so I closed this and opened a new one at #1352 that merges with support/lisf-public-7.4.

Sorry for missing the documentation on this workflow.