-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update to diags handling #633
base: main
Are you sure you want to change the base?
Conversation
9f8e681
to
c3bbb5b
Compare
Checked for each set in |
As far as I can tell, In any case, an explicit dependency check has been added with this PR. |
The min-case test Error description
|
So far so good on my latest version of the cfg from #622 (comment) though: the other two diags tasks are running while the tc_analysis diags task waits for tc_analysis to finish. |
Test the cfg from #622Re: the issues with the cfg at #622 (comment). The cfg (5th iteration from the original cfg)
These links show viewers:
Some sets still need better handlingLooking at the sets used in this cfg, it looks like
That means those 2 sets don't occur in either the Example error from
@chengzhuzhang I'll look into this more, but let me know if anything stands out as immediately wrong (either in how zppy is calling Diags or in Diags itself). Action items to do before merging this PR
|
@chengzhuzhang I need more context for the
|
# min_case_e3sm_diags_depend_on_ts: "enso_diags","qbo", | ||
# min_case_e3sm_diags_diurnal_cycle: "diurnal_cycle", | ||
# min_case_e3sm_diags_streamflow: "streamflow", | ||
# min_case_e3sm_diags_tc_analysis: "tc_analysis", | ||
# min_case_e3sm_diags_tropical_subseasonal: "tropical_subseasonal", | ||
sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","enso_diags","qbo","diurnal_cycle","streamflow","tc_analysis","tropical_subseasonal", | ||
sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","enso_diags","qbo","diurnal_cycle","streamflow","tc_analysis","tropical_subseasonal","aerosol_aeronet","aerosol_budget" |
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.
All 17 sets excluding area_mean_time_series
which I think isn't used much now?
Yes, this is correct
Yes, this is true
This seems to be a diags error. |
@chengzhuzhang I've renamed this PR to better cover that the changes affect a number of sets. We need two changes to the
Thanks! |
Thanks. yes, it makes sense to update the upstream tool first. |
@forsyth2 item one has been taken care of by E3SM-Project/e3sm_diags@ffbdf28. |
The only thing is no plots will generate for the aerosol sets if we run into that bug in the weekly testing too (now that the aerosol sets have been added to weekly testing). But correct, zppy can still function. |
The aerosl_budget table won't be created because the variables are not included in the standard v3 output that zppy weekly tests run with. @forsyth2 sorry I accidentally messed up your comment by editing... |
Issue resolution
tc_analysis
in zppy #622 (specifically the issue at [Bug]: Issues withtc_analysis
in zppy #622 (comment))Select one: This pull request is...
1. Does this do what we want it to do?
Objectives:
lat_lon_land
set (fixes [Bug]: Issues withtc_analysis
in zppy #622 (comment))check_parameter_defined
.tc_analysis
error on empty dat (after the non-empty check is implemented in e3sm_diags instead) (fixes Improve input validation and testing #628 (comment))Required:
If applicable:
2. Are the implementation details accurate & efficient?
Required:
If applicable:
zppy/conda
, not just animport
statement.3. Is this well documented?
Required:
4. Is this code clean?
Required:
If applicable: