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

Change all history fields to use the new landunit_mask argument, and remove initial settings of history fields to spval #1347

Open
billsacks opened this issue Apr 19, 2021 · 1 comment
Labels
blocked: dependency Wait to work on this until dependency is resolved blocker another issue/PR depends on this one enhancement new capability or improved behavior of existing capability

Comments

@billsacks
Copy link
Member

billsacks commented Apr 19, 2021

Once we are satisfied with how we want to move ahead with history field averaging, we should change all history fields to be consistent with our plan.

Tentatively, this will involve changing all history fields to use the new landunit_mask argument and removing the initial setting of each history field to spval. This will also involve removing any uses of set_*=spval, instead using the appropriate setting for landunit_mask to serve that purpose.

In the meeting a couple of weeks ago a suggestion was raised of just starting with default-on biogeophysical variables. However, thinking about this more, I see a couple of issues with this plan:

  • Only addressing default-on variables would make it likely that some default-off variables would be inconsistent with some close companions that are default-on. So I think we should address all biogeophysical variables with similar care.
  • If we address all variables (BGC as well as biogeophysical), then we can do some other code cleanup and add some robustness, such as doing away with initial settings to spval (and any code that checks for that, e.g., in subgridAveMod) and ensuring that all history variables define a landunit_mask moving forward. So I would suggest that we make the same changes to BGC variables. However, we can make this process easier by assuming that the current behavior of BGC variables is correct (in contrast, we discussed giving more critical thought to the biogeophysical variables, which I believe are more likely to be incorrect); in that case, I believe it will be relatively quick to add the new landunit_mask argument to the relevant history fields.

We should test to make sure answers don't change from doing this. Doing this with confidence depends on #29 (blocked by #29).

Once we have done the above, we can – and should:

@billsacks
Copy link
Member Author

Blocks #1349

@billsacks billsacks changed the title Change all history fields to use the new landunit_mask argument, and removing initial settings of history fields to spval Change all history fields to use the new landunit_mask argument, and remove initial settings of history fields to spval Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked: dependency Wait to work on this until dependency is resolved blocker another issue/PR depends on this one enhancement new capability or improved behavior of existing capability
Projects
None yet
Development

No branches or pull requests

1 participant