Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

What is the preferred mechanism for setting a history field to 0 over certain landunits? #1351

Closed
billsacks opened this issue Apr 19, 2021 · 4 comments
Labels
next this should get some attention in the next week or two. Normally each Thursday SE meeting.

Comments

@billsacks
Copy link
Member

One outstanding question from the meeting a couple of weeks ago is: What is the preferred mechanism for setting a history field to 0 over certain landunits? Currently this can be done via:

  • The set_* arguments to hist_addfld
  • Setting to 0 in initCold
  • Setting to 0 every time in the run loop

My (weak) preference is to just use initCold for this purpose and do away with the set_* arguments. My rationale is:

  1. initCold feels like a more explicit and intuitive mechanism for initializing fields: initCold is where I would look to see how a field is initialized over landunits where that field is calculated, so it feels natural to me to find the setting to 0 over other landunits in that same place. (This is especially true for state variables.)
  2. Doing the setting to 0 in the hist_addfld call leaves open the possibility that this would be misleading/wrong, if the field is later set to a different value in initCold or the run loop (see History fields incorrect when set_xxx=0 but the xxx landunit is also set in initCold #431 ).
  3. Similarly, doing the setting to 0 in the hist_addfld call opens the door to subtle bugs like: a field is initialized to 0 over special landunits in hist_addfld; some other code relies on this setting to 0 (e.g., some other diagnostic field is calculated based on this one, with the calculation done for all landunits); then we find that this isn't needed as a history field, so the hist_addfld call is removed – along with its initializations to 0 which were actually needed for some other purposes!

However, I can see arguments for different mechanisms, such as:

  • Doing this initialization in initCold doesn't lend itself to metadata describing where this field is always 0.
  • Doing this initialization in initCold may require more lines of code than some alternatives
  • (And maybe others that I haven't thought of)

I'd like to know what others think here. One important question is: How important do people feel it is to have metadata on where a given field is always 0? I have viewed this as less important than having metadata on where a field is completely undefined (i.e., which landunits an average is taken over), but maybe others see more importance in this?

If this metadata is important, then we could use a mechanism suggested by @djk2120 - in addition to a landunit_mask (formerly l2g_scale_type) argument to hist_addfld, we could also have an argument like landunit_zeros (not sure if that's the best name) which would specify which landunits are set to 0 in the averaging. The difference between this mechanism and the current set_* mechanism is that (in my mind) this mechanism would not actually set the field values, but instead would just dynamically insert 0 values when a history field is averaged up to the grid cell level. Referring to the numbered points in my rationale above, this mechanism would address these points to some extent. However, there are still possibilities for issues with this mechanism:

  • A field could have landunit_zeros='lake' (which says: in the history writing, treat all lake points as zero), but then its cold start initialization could also set the field to zero for glaciers, and then the field may stay fixed at zero for glaciers; in that case, metadata that says the field is only forced to zero over lakes would be misleading. I believe we currently have issues like this for the set_* arguments which makes them an unreliable way to know where a field is actually set to zero. So if people want metadata like this, I'd like to also hear ideas for dealing with this issue, and/or how concerned others are about this issue of misleading metadata.
  • I can see some arguments for actually setting field values to 0 rather than just applying 0s when doing the history averaging. For example, if you have diagnostic quantities that sum across some other fields, then it could be helpful to have those other fields actually set to 0 where you want them 0, so that the diagnostic sums Just Work without needing to know which landunits are set to 0 for each component of the sum. Similarly, if you communicate a field to another component (like the atmosphere) then you need explicit zeros. However, I'm not sure how important these last issues are in practice.

See also #1350

@djk2120 @danicalombardozzi @wwieder @dlawrenncar @olyson and others - I'd like to hear your thoughts on this.

@wwieder
Copy link
Contributor

wwieder commented Apr 20, 2021

@billsacks I appreciate the thought you've put into this project. One point of clarification, are we still at the "I need to focus on other projects and just want to record some notes on averaging" stage, or are you moving deeper into this work? Assuming it's the former I may be brief here.

Using the landunit_zeros='lake' approach seems efficient as it specifies how averaging is being done clearly in the metadata. My only question is if this would potentially introduce confusion if subsequently users were looking at subgrid output and found non-zero values for over lakes for this quantity?

I don't really have thoughts on particulars of initCold vs. other ways of setting zeros over particular landunits, but would be happy to talk this through if you're planning on moving ahead with this work.

@billsacks
Copy link
Member Author

are we still at the "I need to focus on other projects and just want to record some notes on averaging" stage

Yes, this: I had a lot more thoughts floating around in my mind yesterday and I wanted to record them for the sake of others or my future self, whoever ends up working on this project. But I won't have time to work on it myself for a while.

Thanks for sharing some brief thoughts. I'll be honest: The more I think about this, the more nervous I get about an approach like landunit_zeros='lake': I feel like we're going to have some challenges ensuring that the metadata is actually correct for most / all fields in this respect. Moreover, I think it will require a pretty significant overhaul of how things are done for most / all of the BGC variables.

So I think the starting point for this discussion should maybe be the question of how important it is to have metadata on where fields are zero. If this is important, then we can discuss how much extra work (short-term and as a long-term maintenance cost) it's worth doing to get that metadata. If nobody feels it's too important to have that metadata, then we can go with a solution that doesn't provide that metadata. And I guess the latter would still leave the door open to something like landunit_zeros='lake', just with the understanding that you can't necessarily get the complete, correct picture just by looking at that landunit_zeros argument.

@olyson
Copy link
Contributor

olyson commented Apr 21, 2021

I tend to agree that InitCold is the most logical single place to set history fields to zero over certain landunits. Doing so would certainly clean up some of the code in the science subroutines where fields are set to zero, e.g., in LakeHydrologyMod.F90:

   qflx_drain_perched(c) = 0._r8
   qflx_rsub_sat(c)      = 0._r8
   qflx_infl(c)          = 0._r8
   qflx_surf(c)          = 0._r8
   qflx_drain(c)         = 0._r8

It would be nice to have metadata that describes over what landunits the field is set to zero but I don't think it's absolutely essential. Most of the fields that are currently set to zero over lakes for example intuitively make sense, e.g., photosynthesis (fpsn, fpsn_wc, fpsn_wj, fpsn_wp) and hydrology fields (snocan, liqcan). It wouldn't take long for someone to work out why photosynthesis or canopy water is low for a gridcell with lake.

@ekluzek
Copy link
Collaborator

ekluzek commented Aug 14, 2024

Converting this to a discussion.

It sounds like the decision here was that InitCold is preferred, if that's the case should we do anything in the code to improve this?

@ekluzek ekluzek added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Aug 14, 2024
@ESCOMP ESCOMP locked and limited conversation to collaborators Aug 14, 2024
@ekluzek ekluzek converted this issue into discussion #2694 Aug 14, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
next this should get some attention in the next week or two. Normally each Thursday SE meeting.
Projects
None yet
Development

No branches or pull requests

4 participants