zenith angle time correction #123

jedwards4b · 2021-10-01T18:59:47Z

Description of changes

Corrects the calculation of solar zenith angle, which was off by one timestep.

Specific notes

Sean ran two five day simulations, with and without your cdeps changes. The changes correct the bug.

Contributors other than yourself, if any: Sean Swenson

CDEPS Issues Fixes #120 #35

Are there dependencies on other component PRs

CIME (list)
CMEPS (list)

Are changes expected to change answers?

bit for bit
different at roundoff level
more substantial

Any User Interface Changes (namelist or namelist defaults changes)?

Yes
No

Testing performed:

(required) aux_cdeps
- machines and compilers:
- details (e.g. failed tests):
(optional) CESM prealpha test
- machines and compilers
- details (e.g. failed tests):

Hashes used for testing:

CIME
- repository to check out: https://github.com/ESCOMP/CESM.git
- branch:
- hash:
CMEPS
- repository to check out: https://github.com/ESCOMP/CESM.git
- branch:
- hash:
CESM
- repository to check out: https://github.com/ESCOMP/CESM.git
- branch:
- hash:

billsacks · 2021-10-01T19:15:28Z

Thanks a lot for helping to push this forward, @jedwards4b . I think more appropriate reviewers here would be @ekluzek and @swensosc so I'm going to request reviews from them instead of me.

In terms of documenting answer changes: my understanding is that this is more than roundoff-level changes; @swensosc, is that correct?

We had also talked about getting sign-off from people in the ocean group (@mnlevy1981 @alperaltuntas @klindsay28 or others) and sea ice (@dabail10 ), so I'll add them as possible reviewers.

All: feel free to remove yourself or suggest someone else if you feel someone else would be a more appropriate reviewer.

jedwards4b · 2021-10-01T19:18:25Z

Here are plots provided by @swensosc
cdeps_coszen_fix.pdf

swensosc · 2021-10-01T19:22:07Z

yes, I expect this is answer changing, because it is changing the incoming solar radiation when the coszen interpolation is used. I agree that it would be best to have others who use this time interpolation to review and sign off on it before bringing it to main.

…

On Fri, Oct 1, 2021 at 1:15 PM Bill Sacks ***@***.***> wrote: Thanks a lot for helping to push this forward, @jedwards4b <https://github.com/jedwards4b> . I think more appropriate reviewers here would be @ekluzek <https://github.com/ekluzek> and @swensosc <https://github.com/swensosc> so I'm going to request reviews from them instead of me. In terms of documenting answer changes: my understanding is that this is more than roundoff-level changes; @swensosc <https://github.com/swensosc>, is that correct? We had also talked about getting sign-off from people in the ocean group ( @mnlevy1981 <https://github.com/mnlevy1981> @alperaltuntas <https://github.com/alperaltuntas> @klindsay28 <https://github.com/klindsay28> or others) and sea ice ***@***.*** <https://github.com/dabail10> ), so I'll add them as possible reviewers. All: feel free to remove yourself or suggest someone else if you feel someone else would be a more appropriate reviewer. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGRN57GM3EF2QAHIUBT7RYDUEYCF3ANCNFSM5FFJXYQA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

ekluzek · 2021-10-01T21:33:31Z

streams/dshr_strdata_mod.F90

@@ -1255,7 +1255,7 @@ subroutine shr_strdata_readLBUB(sdat, ns, mDate, mSec, newData, istr, rc)
    call ESMF_TraceRegionExit(trim(istr)//'_setup')

    ! if model current date is outside of model lower or upper bound - find the stream bounds
-    if (rDateM < rDateLB .or. rDateM > rDateUB) then
+    if (rDateM < rDateLB .or. rDateM >= rDateUB) then


I've convinced myself this change is correct. Because, it will update the data when rDateM == rDateUB. So this should be done in preparation of that.

ekluzek · 2021-10-01T21:47:39Z

streams/dshr_tinterp_mod.F90

       !--- advance to next time in [LB,UB] ---
       ymd0 = ymd
       tod0 = tod
       reday0 = reday
       call shr_cal_advDateInt(ldt,'seconds',ymd0,tod0,ymd,tod,calendar)
       call shr_cal_timeSet(reday,ymd,tod,calendar)

-       if (reday > reday2) then


OK, looking at this, the reason the previous code is wrong is that it's essentially doing the interval (LB,UB] so not starting at ldt after LB (Lower bound) (so LB is exclusive) and continuing until UB (Upper Bound) inclusive. So LB exclusive and UB inclusive. But, once you reach the UB, you go to the next interval. So it should be [LB,UB) LB inclusive, and UB exclusive, which is what the new code changes do.

ekluzek

I approve this, but as it's answer changing for several components, we need to bring it in as it's own tag in CESM where answer changes are allowed.

ekluzek · 2021-10-01T22:00:55Z

@mnlevy1981 @dabail10 and @alperaltuntas can you review this? I'd also like to hear if you agree with the comments I added where I explain why I thought the new changes are correct.

@swensosc can you explain your plots that Jim included a little further? It looks like the magenta plots are the current behavior in pages 3 and 5. If I'm reading it right then pages 4 and 6 are the new behavior. But, what's black and blue correspond to? They looked more displaced to me, so why is this better?

swensosc · 2021-10-04T13:29:40Z

on page 4 the blue is the three hourly forcing. The time step forcing should change when the three hourly forcing changes. The black is the new behavior and the magenta is the old behavior on page 4 also. It shows that the individual sections of the coszen interpolation are shifted by a time step for the new vs old behavior, and that the new behavior leads to the interpolation changing at the same time as the forcing data, while the old behavior is delayed by a time step.

…

On Fri, Oct 1, 2021 at 4:01 PM Erik Kluzek ***@***.***> wrote: @mnlevy1981 <https://github.com/mnlevy1981> @dabail10 <https://github.com/dabail10> and @alperaltuntas <https://github.com/alperaltuntas> can you review this? I'd also like to hear if you agree with the comments I added where I explain why I thought the new changes are correct. @swensosc <https://github.com/swensosc> can you explain your plots that Jim included a little further? It looks like the magenta plots are the current behavior in pages 3 and 5. If I'm reading it right then pages 4 and 6 are the new behavior. But, what's black and blue correspond to? They looked more displaced to me, so why is this better? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGRN57ACA4UYDFGOYYVS2DDUEYVSHANCNFSM5FFJXYQA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

alperaltuntas

I confirm that this PR resolves the issues I posted earlier:
#35
ESMCI/cime#3802

ekluzek · 2021-10-04T19:46:45Z

@swensosc thanks for that explanation. Now, that I understand the plots I agree that the new behavior is better than the old. @alperaltuntas confirmation that it fixes an issue he found as well, confirms this is correct even more.

mvertens · 2022-01-06T19:17:12Z

@dabail10 @mlevy - could you please review this PR so that it can be merged. I have already checked with @swensosc and he is comfortable with going ahead with the merge once all reviews are in.

dabail10

Do we have a unit test of this to print out the values at a particular lat/lon? It generally looks good to me.

mnlevy1981 · 2022-01-06T22:22:28Z

@klindsay28 and I looked through this code, and have some experiments in mind to determine the extent to which this will change answers in POP... but given @alperaltuntas's assessment that it fixes a bug with the coszen implementation, it seems likely that the result of those experiments will be to let others know what does / does not change with this fix in place rather than raise issues with the code changes themselves.

…zenith_angle_correction" This reverts commit 39f79b4, reversing changes made to 4ebc156.

Revert PR #123 ### Description of changes PR #123 (solar zenith angle correction) led to exact restart failures in some situations: see #144. This PR backs out the changes from #123 . ### Specific notes Contributors other than yourself, if any: CDEPS Issues Fixed (include github issue #): Temporarily deals with #144. Are there dependencies on other component PRs (if so list): no Are changes expected to change answers (bfb, different to roundoff, more substantial): more substantial. I have verified that this brings answers back to being bfb with those in cdeps0.12.35 (i.e., prior to #123) via a run of the aux_clm test suite. Any User Interface Changes (namelist or namelist defaults changes): no Testing performed (e.g. aux_cdeps, CESM prealpha, etc): aux_clm Hashes used for testing: billsacks/ctsm@3fd7f6fbf

jedwards4b added 2 commits September 30, 2021 11:17

zenith angle time correction

593566d

correct compensating error

4f99c9f

jedwards4b requested a review from billsacks October 1, 2021 19:00

jedwards4b self-assigned this Oct 1, 2021

billsacks requested review from ekluzek, mnlevy1981, alperaltuntas, dabail10 and swensosc and removed request for billsacks October 1, 2021 19:15

jedwards4b mentioned this pull request Oct 1, 2021

Bug in coszen time interpolation in shr code #35

Closed

ekluzek added the bug Something isn't working label Oct 1, 2021

ekluzek reviewed Oct 1, 2021

View reviewed changes

ekluzek approved these changes Oct 1, 2021

View reviewed changes

alperaltuntas approved these changes Oct 4, 2021

View reviewed changes

ekluzek mentioned this pull request Oct 4, 2021

Bug in coszen time interpolation in shr code ESMCI/cime#3802

Closed

rljacob mentioned this pull request Oct 4, 2021

Cosine solar zenith angle forcing is off by a time-step in data models E3SM-Project/E3SM#4575

Closed

jonbob mentioned this pull request Oct 7, 2021

Corrects the calculation of solar zenith angle, which was off by a timestep E3SM-Project/E3SM#4589

Merged

Merge branch 'master' into jedwards/solar_zenith_angle_correction

70009ce

mvertens mentioned this pull request Jan 6, 2022

Cosine solar zenith angle forcing is off by a time-step #120

Closed

Merge branch 'master' into jedwards/solar_zenith_angle_correction

11a48c8

jedwards4b requested a review from mvertens January 6, 2022 19:22

jedwards4b removed request for mnlevy1981, dabail10 and swensosc January 6, 2022 19:23

mvertens approved these changes Jan 6, 2022

View reviewed changes

mnlevy1981 self-requested a review January 6, 2022 19:38

jedwards4b merged commit 39f79b4 into ESCOMP:master Jan 6, 2022

jedwards4b deleted the jedwards/solar_zenith_angle_correction branch January 6, 2022 19:38

mnlevy1981 removed their request for review January 6, 2022 19:38

dabail10 reviewed Jan 6, 2022

View reviewed changes

billsacks added a commit to billsacks/CDEPS that referenced this pull request Mar 15, 2022

Revert "Merge pull request ESCOMP#123 from jedwards4b/jedwards/solar_…

bae31f4

…zenith_angle_correction" This reverts commit 39f79b4, reversing changes made to 4ebc156.

This was referenced Mar 15, 2022

Revert PR #123 #148

Merged

Solar zenith angle changes cause some exact restart tests to fail #144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zenith angle time correction #123

zenith angle time correction #123

jedwards4b commented Oct 1, 2021 •

edited

Loading

billsacks commented Oct 1, 2021

jedwards4b commented Oct 1, 2021 •

edited

Loading

swensosc commented Oct 1, 2021 via email

ekluzek Oct 1, 2021

ekluzek Oct 1, 2021

ekluzek left a comment

ekluzek commented Oct 1, 2021

swensosc commented Oct 4, 2021 via email

alperaltuntas left a comment

ekluzek commented Oct 4, 2021

mvertens commented Jan 6, 2022

dabail10 left a comment

mnlevy1981 commented Jan 6, 2022

zenith angle time correction #123

zenith angle time correction #123

Conversation

jedwards4b commented Oct 1, 2021 • edited Loading

Description of changes

Specific notes

billsacks commented Oct 1, 2021

jedwards4b commented Oct 1, 2021 • edited Loading

swensosc commented Oct 1, 2021 via email

ekluzek Oct 1, 2021

Choose a reason for hiding this comment

ekluzek Oct 1, 2021

Choose a reason for hiding this comment

ekluzek left a comment

Choose a reason for hiding this comment

ekluzek commented Oct 1, 2021

swensosc commented Oct 4, 2021 via email

alperaltuntas left a comment

Choose a reason for hiding this comment

ekluzek commented Oct 4, 2021

mvertens commented Jan 6, 2022

dabail10 left a comment

Choose a reason for hiding this comment

mnlevy1981 commented Jan 6, 2022

jedwards4b commented Oct 1, 2021 •

edited

Loading

jedwards4b commented Oct 1, 2021 •

edited

Loading