-
Notifications
You must be signed in to change notification settings - Fork 138
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
Metadata fix #45
Metadata fix #45
Conversation
Since the NWM routine was using the timestep of the hydro model, the LSM outputs time stamp was not right in cases that the time steps of LSM and HYDRO differs. I added ITIME to the routine, to be able to calculate the correct time in the output routines.
…ribute calcualted the time in the LSM output routine based on the LSM time step and not the HYDRO timestep. Also fixed the valid_max to present the time of the last exising file and not the end of the simulation.
Waiting for private pull request queue to clear. |
Authentication required - Linked to private pull request #197, link: https://github.com/NCAR/wrf_hydro_nwm/pull/197 |
Pull request submitted to private repo. Waiting for reviewer(s) to approve. |
There is no comment that the PR passes any kind of regression test. |
Given that the restart files all check and the CHRTOUT and CHANOBS files fail, I'm going to assume those differences are meta-data only. |
@@ -1518,7 +1523,7 @@ subroutine land_driver_ini(NTIME_out,wrfits,wrfite,wrfjts,wrfjte) | |||
print*, "t0OutputFlag: ", t0OutputFlag | |||
#endif | |||
!#ifdef HYDRO_REALTIME | |||
if(t0OutputFlag .eq. 1) call ldas_output() | |||
if(t0OutputFlag .eq. 1) call ldas_output(0) |
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 love how this is so opaque. What was the assumed value when nothing was passed, what does 0 represent. Is there no good variable that can be used?
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.
hmm, that is the initialization timestep, actually I could use ITIME as I used it in the other routines. I will check why I did not use it, and it possible I would replace it with ITIME.
subroutine ldas_output() | ||
subroutine ldas_output(itime) | ||
integer, intent(in) :: itime ! time step of the LSM, had to explicitly declare it since | ||
! it did not exist in the land_driver_ini |
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.
👍 The comment is good, but notice how this kind of comment goes stale "had to explicitly..."
I love keywords. I wish they were slightly easier to use in fortran.
|
||
! calculate total_valid_time | ||
fileMeta%totalValidTime = int(nlst_rt(1)%khour * 60 / nlst_rt(1)%out_dt) ! # number of valid time (#of output files) | ||
|
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 get why these time calculations are here. I would just like to point out that these kinds of calculations should go in a module_time.F and the results should be requested here (not calculated).
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.
This PR needs a bit more explicit description of what the changes were. @arezoorn please correct if any of this is wrong: "I have added the min_valid and max_valid as attributes to the output files from NWM output routines. " Specifically, in module_NWM_io_dict:
These are now metadata attributes in the CHANOBS and CHRTOUT (same in both) with the following names:
|
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.
See my comments. I'm approving with a lot of noise and whining. 🎊
Definitely agree that it would be nice to have some additional info regarding test failures in the log. Just ran the tests again offline to confirm (file diffs below) and as expected we just see differences in the attributes. CHRTOUT
|
@arezoorn |
Katelyn, thanks for doing that. I know it's currently a pain. should be less painful soon!! |
Added model configuration to compile options json
I have added the min_valid and max_valid as attributes to the output files from NWM output routines. To do so, I added KHOUR to the nlst_rt, since the simulation duration was not accessible in other routines.
I also added one global attribute called model_total_valid_times which is basically the total number of outputs for a given file format (for example CHRTOUT) during the simulation.
I also resolved NCAR/wrf_hydro_nwm-issues#139 . The NoahMP time stamp was wrong (when the time stamp between LSM and Hydro differed). I added ITIME which is the LSM time step to the output routine and used that instead of out_counts which is Hydro time step. I also ran the model with LSM only, checked that the time is correct.
There was also a non reported issue/bug that is fixed with this change, the time variable was wrong if HYDRO was active and the LSM output timestep was not a multiplicative of the Hydro output timestep. For example if user was setting the LSM output timestep to 36000 (10hours) and Hydro output timestep to 180 (3 hours) the time variable in the LSM output was wrong. This was not a reported bug, since in our operational runs and our tests, we usually define the LSM output timestep a miltuplicative factor of the LSM output.