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

Reorder diags sets #521

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Reorder diags sets #521

merged 1 commit into from
Oct 12, 2023

Conversation

forsyth2
Copy link
Collaborator

Reorder diags sets by 1) core or speciality and then 2) older to newer, rather than the alphabetical order introduced in #458. The alphabetical order introduced an error to E3SM Diags: E3SM-Project/e3sm_diags#738.

Benefits of alphabetical order (introduced in #458):

  • Improved readability/usability. Much easier to see if you're including all the sets you wanted to. Much easier for people viewing the diagnostics to locate the set they want to view.
  • Makes it clear where a set should be inserted. (e.g., diurnal_cycle isn't included in sets by default, so at which point in the list should it be added if you want it?)

Benefits of custom order (before #458, again after this pull request):

  • Some sets (notably lat_lon) must be run earlier.
  • We want the sets most people use (the core set) to be listed first.
  • People are used to the current order.

@forsyth2 forsyth2 added the semver: bug Bug fix (will increment patch version) label Oct 11, 2023
@forsyth2 forsyth2 self-assigned this Oct 11, 2023
@@ -188,7 +188,10 @@ run_type = string(default="model_vs_obs")
# "qbo" requires `ref_final_yr` to be set.
# "streamflow" requires `streamflow_obs_ts` to be set.
# "tc_analysis" requires `tc_obs` to be set.
sets = string_list(default=list("aerosol_aeronet","aerosol_budget","annual_cycle_zonal_mean","cosp_histogram","lat_lon","meridional_mean_2d","polar","zonal_mean_2d","zonal_mean_2d_stratosphere","zonal_mean_xy"))
# The order of the `sets` list is the order the sets will show up in E3SM Diags.
Copy link
Collaborator Author

@forsyth2 forsyth2 Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the above comments listing sets in alphabetical order, for ease of readability.

@@ -1,5 +1,5 @@
[e3sm_diags]
sets = "aerosol_aeronet","aerosol_budget","annual_cycle_zonal_mean","cosp_histogram","diurnal_cycle","enso_diags","lat_lon","meridional_mean_2d","polar","qbo","streamflow","zonal_mean_2d","zonal_mean_2d_stratosphere","zonal_mean_xy"
sets= "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","enso_diags","qbo","diurnal_cycle","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","aerosol_aeronet","aerosol_budget"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"aerosol_aeronet","aerosol_budget" throughout this pull request is why we can't just revert #458 -- doing so would remove those two sets from all these lists.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"streamflow" is missing

@forsyth2
Copy link
Collaborator Author

@chengzhuzhang @mahf708 Re: E3SM-Project/e3sm_diags#738, this undoes most of #458, while keeping the addition of the two new aerosol sets. Please let me know if the ordering here is acceptable.

@forsyth2 forsyth2 mentioned this pull request Oct 11, 2023
Copy link

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @forsyth2! My understanding from @chengzhuzhang is that having lat_lon first is the most important part and your ordering nows is perfect to fix the issue with the htmls being misplaced. Thank you!!!

Copy link
Collaborator

@chengzhuzhang chengzhuzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the cryo .cfg missing one set. Other than that, the PR looks good to me. I guess we can have a patch release of zppy for e3sm_unified patch release.

@forsyth2 forsyth2 merged commit 2fbc427 into main Oct 12, 2023
@forsyth2 forsyth2 deleted the reorder-diags branch October 12, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: bug Bug fix (will increment patch version)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants