-
Notifications
You must be signed in to change notification settings - Fork 45
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
fixes an issue in aux data streams #193
Conversation
@fischer-ncar could you please run prealpha tests with this branch. |
Can do. |
I think that the issue here is that the cycle method in the model is only designed to cycle over full years of data and we are asking for a cycle over 2 days worth of data. |
Exactly
…On Mon, Nov 7, 2022 at 7:05 AM Jim Edwards ***@***.***> wrote:
I think that the issue here is that the cycle method in the model is only
designed to cycle over full years of data and we are asking for a cycle
over 2 days worth of data.
—
Reply to this email directly, view it on GitHub
<#193 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4XCE5KJADBXA6C7GNQCETWHEEEHANCNFSM6AAAAAARVPD6IE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
I guess I wasn't aware of that - I thought that we were trying to fix a bug in current functionality when in fact we want to add new functionality. |
Sorry i didn’t get a chance to help with this. Your bug fix is still
needed?
…On Mon, Nov 7, 2022 at 8:10 AM Jim Edwards ***@***.***> wrote:
I guess I wasn't aware of that - I thought that we were trying to fix a
bug in current functionality when in fact we want to add new functionality.
—
Reply to this email directly, view it on GitHub
<#193 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4XCEYTDKTSXRFV7CFSFMLWHELXLANCNFSM6AAAAAARVPD6IE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
That change broke all of our existing tests - I spent some time this weekend trying to better understand this code and rewriting a lot of the date and time variables as ESMF derived types. I think I have a better handle on the problem now and should have a solution soon. I do have questions about what new functionality should be allowed - for example if we provide several days input data we want to cycle over those full days - what if only 6 hours of inputdata is provided? I assume we do not want to cycle over that and should generate an error since it would not have a diurnal cycle. |
Now that I understand this to be new functionality and not a bug fix I have found a solution. For streams that do I am not including this variable in the defaults, I think that in order to make sure that the user is aware that we are bypassing the limit they must set it explicitly for each stream file in user_nl_d*_streams |
@fischer-ncar Could you please run prealphas with this branch? |
prealpha testing looks goog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also tested this under UFS by running couple of data atmosphere configurations and all worked fine.
@@ -779,7 +779,10 @@ subroutine datm_init_dfields(rc) | |||
strm_flds2 = (/'Faxa_ndep_nhx', 'Faxa_ndep_noy'/) | |||
call dshr_dfield_add(dfields, sdat, trim(lfieldnames(n)), strm_flds2, exportState, logunit, mainproc, rc) | |||
if (ChkErr(rc,__LINE__,u_FILE_u)) return | |||
|
|||
case('cpl_scalars') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just required for data atmosphere? Do you need to same for other data components? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for asking. The other components either use the explicit fieldname in the call or already skip the cpl_scalar fields.
Description of changes
The calculation of ddateLB seems to be different in different parts of the file, at line 965 we have
yy = yy + (mYear-dYear-nYears) while at line 1154 we had
yy = yy + (mYear-dYear) changing the line at 1154 to match that at 965 seems to have solved the issue.
Specific notes
Contributors other than yourself, if any:
CDEPS Issues Fixed (include github issue #):
Are there dependencies on other component PRs (if so list):
Are changes expected to change answers (bfb, different to roundoff, more substantial):
Any User Interface Changes (namelist or namelist defaults changes):
Testing performed (e.g. aux_cdeps, CESM prealpha, etc):
Hashes used for testing: