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

oldfflag can NOT be used with subgridflag==1, and rename subgridflag #502

Closed
ekluzek opened this issue Sep 6, 2018 · 4 comments · Fixed by #769
Closed

oldfflag can NOT be used with subgridflag==1, and rename subgridflag #502

ekluzek opened this issue Sep 6, 2018 · 4 comments · Fixed by #769
Assignees
Labels
type: enhancement new capability or improved behavior of existing capability

Comments

@ekluzek
Copy link
Contributor

ekluzek commented Sep 6, 2018

Brief summary of bug

In looking at the code in a ctsm meeting we noticed that oldflag can NOT be used with subgridflag==1.

General bug information

CTSM version you are using: ctsm1.0.dev010

Does this bug cause significantly incorrect results in the model's science? Yes

Used together results wouldn't make sense.

Configurations affected: Clm45 case turning subgridflag=1 and oldfflag=1

Details of bug

This configuration will probably run, but give screwy results, because snow_depth will be set by frac_snow of the other method, and then frac_snow will be overwritten by the oldffflag method. We just decided this based on code inspection, without doing a simulation.

We also decided that oldfflag is a flag that should be used for the NWP case.

@ekluzek ekluzek self-assigned this Sep 6, 2018
@billsacks billsacks added type: enhancement new capability or improved behavior of existing capability and removed type: bug - impacts science labels Sep 6, 2018
@billsacks
Copy link
Member

@ekluzek I'm relabeling this as an enhancement because this is really just about adding an error-check on user inputs. My feeling is that "bug - impacts science" should really be restricted to areas where the model is giving scientifically incorrect answers for some relatively common configuration. That, in my mind, labels a serious scientific issue that needs to be addressed, and I feel we should try not to dilute that label too much.

@billsacks
Copy link
Member

While doing this, we should rename subgridflag to use_subgrid_fluxes, as @swensosc suggested.

@billsacks
Copy link
Member

@ekluzek you self-assigned this, but I'd be happy to take this on in the course of doing my CanopyHydrology cleanup. So I'll reassign it to myself.

@billsacks billsacks assigned billsacks and unassigned ekluzek Sep 6, 2018
@billsacks billsacks changed the title oldfflag can NOT be used with subgridflag==1 oldfflag can NOT be used with subgridflag==1, and rename subgridflag Sep 6, 2018
@billsacks billsacks added this to To Do in Water isotopes Sep 6, 2018
@billsacks billsacks moved this from To Do to In progress in Water isotopes Apr 24, 2019
billsacks added a commit to billsacks/ctsm that referenced this issue Jul 10, 2019
These options are incompatible.

Partially addresses ESCOMP#502
billsacks added a commit to billsacks/ctsm that referenced this issue Jul 10, 2019
Also, fix error checks involving use_subgrid_fluxes in CLMBuildNamelist:
previously, subgridflag was not set at the point when the error checks
involving that value were being done; I have fixed that by always
including use_subgrid_fluxes in the default namelist.

Resolves ESCOMP#502
@billsacks
Copy link
Member

I noticed that none of the error checks in CLMBuildNamelist involving subgridflag were working as intended: subgridflag wasn't being added to the default namelist, so the error checks only kicked in if the user explicitly specified subgridflag in user_nl_clm. I have fixed that in d8d2a2d by calling add_default for use_subgrid_fluxes. However, it looks like the same problem (impotent error checks) might exist for other variables checked in setup_logic_hydrology_switches (I'm not fixing those for now).

@billsacks billsacks moved this from In progress to Done but not on master in Water isotopes Jul 10, 2019
Water isotopes automation moved this from Done but not on master to Done Aug 6, 2019
billsacks added a commit that referenced this issue Aug 6, 2019
Modularize snow cover fraction method

This tag moves the calculation of frac_sno - and the related updates of
snow_depth - into a new set of classes, with one class for each
parameterization (Niu & Yang 2007 and Swenson & Lawrence 2012).

Previously, the code always calculated frac_sno the new way, but then
possibly overwrote it if using the older Niu & Yang method. The new code
cleans this up, only doing the calculations that are needed for each
method.

In addition, other code that is specific to one of the two methods is
now moved to a home that makes this dependence on method explicit. This
includes the addition of newsnow to int_snow: previously, int_snow was
always updated using an equation specific to the newer CLM5
parameterization of frac_sno, which was not appropriate if using the Niu
& Yang parameterization; this doesn't make a difference currently, since
int_snow is only referenced if using the Swenson & Lawrence
parameterization, but this clears up some confusion. Also, time-constant
parameters read from namelist or the netCDF parameter file now reside in
the appropriate class rather than being more global.

This tag also renames two namelist options to increase clarity:
- subgridflag is renamed to use_subgrid_fluxes, and is now a logical
- oldfflag is renamed to snow_cover_fraction_method, and is now a string

Resolves #502
Resolves #503
Resolves #571
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement new capability or improved behavior of existing capability
Projects
Development

Successfully merging a pull request may close this issue.

2 participants