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

Division of critical day length by 2 is incorrect #1427

Closed
ekluzek opened this issue Jul 15, 2021 · 10 comments · Fixed by #1164
Closed

Division of critical day length by 2 is incorrect #1427

ekluzek opened this issue Jul 15, 2021 · 10 comments · Fixed by #1164
Labels
documentation additions or edits to user-facing documentation
Milestone

Comments

@ekluzek
Copy link
Collaborator

ekluzek commented Jul 15, 2021

This comes from this comment in a previous PR...

#947 (comment)

It refers to this section of code...

        ! Note: The divide by 2 of crit_dayl is just to make sure
        ! onset doesn't happen with less than 6 hours of daylight
        else if (season_decid_temperate == 0 .and.  onset_gddflag == 1.0_r8 .and. &
                soila10 > SHR_CONST_TKFRZ .and. &
                t_a5min > SHR_CONST_TKFRZ .and. ws_flag==1.0_r8 .and. &
                dayl>(crit_dayl/2.0_r8) .and.  snow_5day<snow5d_thresh_for_onset) then

The problem is that the description for the reason for the divide by 2 is incorrect, as it allows onset to happen at 5 hours of daylight rather than 10. The description makes it sound like it makes it harder when it's actual function is to make it easier for onset to happen.

@ekluzek ekluzek added tag: bug - impacts science bug something is working incorrectly next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Jul 15, 2021
@wwieder wwieder added this to the ctsm5.1.0 milestone Jul 15, 2021
@wwieder
Copy link
Contributor

wwieder commented Jul 15, 2021

@ekluzek can you remind me how crit_dayl is defined, is it just being read off the parameter file?
If Leah's intent was to make onset happen with > 6 hours of day light, should we just hard code this in (e.g. dayl(g)>6.0_r8)?

@billsacks
Copy link
Member

Feeling from ctsm-software meeting: there should be a new parameter used in this place – set to what crit_dayl/2 currently gives – so that the two can be varied independently. To do it right, the current parameter should be renamed to crit_dayl_offset.

@billsacks billsacks removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jul 22, 2021
@wwieder
Copy link
Contributor

wwieder commented Jul 22, 2021

I think for this section of code the new parameter here would be crit_dayl_onset, so

  • the logical would be dayl>crit_dayl_onset
  • I'd also suggest crit_dayl_onset = 6 on the parameter file, assuming we can confirm this with @lmbirch89

@wwieder
Copy link
Contributor

wwieder commented Jul 26, 2021

I'm pasting this email from @lmbirch89 for reference:

Thanks so much for continuing to work on this! My apologies for leaving this out of the Supplement; I did add this comment, I believe. It was just a quick threshold that doesn't affect much of the majority of the Arctic-Boreal. In coastal grid cells bordering the Arctic Ocean, some grasses would begin photosynthesizing at the end of January when there was virtually no light and then burn out by summer. With it being frozen and dark, there is likely a model land/sea interaction that was causing onset snow and temperature thresholds to be passed, but I couldn't untangle why. A 6hr daylight threshold fixed the problem and then photosynthesis wouldn't try to start until June, even though the 6 hour daylight threshold is passed in March. I hope this helps. Let me know if you have any other questions.
Thanks,
Leah

@ekluzek can we:

  • create this parameter (crit_dayl_onset = 6 ) and
  • implement it directly in the code (dayl>crit_dayl_onset)

@ekluzek ekluzek added documentation additions or edits to user-facing documentation and removed tag: bug - impacts science bug something is working incorrectly labels Jul 27, 2021
@ekluzek
Copy link
Collaborator Author

ekluzek commented Jul 27, 2021

@wwieder unless we can show that the current value gives identical results to 6hours I suggest we leave it at the current value. It most likely doesn't matter if it's 5 or 6, which lends me to think since we've done tons of simulations with the current value of 5 that we should leave it alone. But, I could also run the test suite and verify that 6 gives the same results as 5.

I think the crit_dayl_onset parameter should be hardcoded as a parameter in CNPhenologyMod.F90. I'm viewing this not as a bug anymore, but as a documentation and code clarity issue.

@wwieder
Copy link
Contributor

wwieder commented Jul 27, 2021

I agree, it makes sense to leave crit_dayl_onset to 5 to avoid changing answers for now, but I guess I still have a few questions:

  • Is there a reason you want to leave this in CNPhenology instead of putting it on the parameter file?
  • Is there any reason to test the effects of changing this value to 6, as Leah intended?
    I'd suspect that regardless, this is a low priority issue for now...

@lmbirch89
Copy link
Contributor

I don't think 6 hours is a hard limit that you have to hit. I should have edited the comment to reflect the real value that I settled on, so I think that would be sufficient here. Like I said, it was merely to stop some odd behavior in January/early February on the coast that doesn't make physical sense. I wouldn't expect 6 hours to have too much of an impact because before I put in this limit, it was very obvious that deciduous plants were turning on in January/February along the coasts.

@ekluzek
Copy link
Collaborator Author

ekluzek commented Jul 27, 2021

@wwieder I included some of my reasoning in our email discussion with @lmbirch89 so I'll quote that here and also expand upon it a bit.

We were planning on making the value used here a parameter on the params file, but since it doesn't work like a regular parameter (changing it changes results immediately) I don't think we should. I think it should be a hard-coded parameter in CNPhenology and set to the current value of 5-hours that's being used. I'll also change the comment to reflect what you just said, and have you look it over to make sure it's accurate. If we put it on the paramfile it would be a parameter that normally wouldn't have any effect at all, and the only cases that it would matter would be if you gave it degenerate values that cause bad results. So that's something that should be hardcoded rather than on the paramsfile.

To expand further the reason to put something on the paramsfile is for something that's a parameter that needs to be tuned for a model parameterization. Or it might be something that has some empirical observations for, but it's got big error bars around it so you need to tune it to get good results. It should also be something that you could modify with machine learning and get sensible results from tuning it. You want it on the paramfile to make it easy to modify and tune results with.

Things that you probably don't want on the parameter file are things that aren't above. So it's something that's observed to a high degree of accuracy for example. So physical constants we have hard coded as parameters in files specific to CTSM or to CESM. In this case this is a "degenerate parameter" because it wont' actually change results whatsoever for midrange values -- but it will create degenerate results for bad values that are either too low or too high. That's the kind of parameter that would wreck a machine learning algorithm, and there's no reason to include it, because there's nothing good about having it in. In this specific case if you make it too low, you'll see the deciduous plants turning on in arctic coasts in January which is degenerate behavior that you don't want to see. But, if you make it too high, arctic deciduous plants won't turn on at all at certain locations which is another kind of degenerate behavior. But, in the middle of those two extreme's -- it won't change answers whatsoever. So that's the kind of degenerate parameter that you don't want users to have easy access to -- because the only thing they can do with by changing it -- is to get the model to give bad results. That's the kind of parameter you want to make it hard for the user to change -- NOT easy to change.

As such I can't think of a legit reason to do any testing with it -- especially with @lmbirch89 comment above. If you did have some kind of observational evidence that does tie leaf onset to day length, maybe we would want to do some more testing. And it's also possible that you might actually see some different behavior for lower values of it (before getting to the degenerate case) that could be useful. Making it higher though is just going to possibly mean that some arctic regions won't allow leaf onset, so that seems problematic to me. So I don't see the utility in doing testing for this.

The only thing I do wonder about is if this parameter will cause problems for paleo or future scenario cases. It seems to me the tuning/testing that might be appropriate here would be to find the smallest value that physiologically makes sense for leaf onset. Or if there is any experimental work out there somewhere that can show that leaf onset can only happen with X number of hours of daylight -- then you might want to set it to that value and test to make sure it seems to work.

@billsacks
Copy link
Member

I think @ekluzek 's rationale above makes a lot of sense. My summary takeaway is: If a parameter just exists as a workaround for some relatively infrequent issue, and shouldn't normally have any effect, then putting it on the parameter file can be more misleading and error-prone than helpful.

@wwieder
Copy link
Contributor

wwieder commented Jul 28, 2021

Sounds good. Thanks for the explanation @ekluzek .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation additions or edits to user-facing documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants