-
Notifications
You must be signed in to change notification settings - Fork 34
Extend snapshot support for other core sets and doc updates #1013
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
Conversation
This commit updates the documentation to reflect new features and
parameters added in v3.1.0:
1. Updated available-parameters.rst:
- Added time_slices parameter for snapshot analysis
- Added lat_lon_native set-specific parameters (test_grid_file,
ref_grid_file, antialiased)
- Added test_file and ref_file parameters
- Updated regrid_tool default from 'esmf' to 'xesmf'
- Updated granulate default to include 'time_slices'
- Updated sets default list to include all current diagnostic sets
- Added notes about mutual exclusivity of time_slices and seasons
2. Updated examples.rst:
- Added Example 8: Native Grid Visualization
- Added Example 9: Snapshot Analysis for Core Sets
- Updated running instructions to include new examples
- Updated batch script example with ex8 and ex9
3. Created example files:
- examples/ex8-native-grid-visualization/ (ex8.py, diags.cfg, README.md)
- examples/ex9-snapshot-analysis/ (ex9.py, diags.cfg, README.md)
These changes document the two major features introduced in v3.1.0:
- Native grid visualization using UXarray
- Snapshot analysis for core diagnostic sets using time_slices
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
|
@tomvothecoder This PR extended the snapshot support implemented in lat_lon_native to other core sets. As well as the documentation update for v3.1.0. Could you please review. Things to review include if the code updates are reasonable, also for documentation update, could you test the added examples ex8 (native-grid-visualization) and ex9 (snap-shot-analysis) following the READme in each folder? Each example has command-line option and script options. Let's try to get this in a new release candidate for Unified. Thanks! |
|
Sounds good @chengzhuzhang. I will review today. |
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.
Pull Request Overview
This PR extends the snapshot analysis feature from lat_lon_native to six additional core diagnostic sets, enabling time slice-based analysis across multiple diagnostic types. The implementation introduces shared utilities for time slice handling and updates data loading infrastructure to support both climatological and snapshot modes.
Key Changes
- Added time slice support to 6 core diagnostic sets (lat_lon, polar, zonal_mean_xy, zonal_mean_2d, meridional_mean_2d, zonal_mean_2d_stratosphere)
- Created shared utility module
driver/utils/time_slice.pyfor common time slice functionality - Enhanced
Dataset.get_climo_dataset()with time slice mode viais_time_sliceparameter
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| e3sm_diags/parameter/core_parameter.py | Added time_slices attribute and granulation support |
| e3sm_diags/parser/core_parser.py | Added --time_slices CLI argument |
| e3sm_diags/driver/utils/time_slice.py | New shared utilities for validation and attribute setting |
| e3sm_diags/driver/utils/dataset_xr.py | Enhanced data loading with time slice support |
| e3sm_diags/driver/*_driver.py | Implemented time slice pattern in 6 diagnostic drivers |
| e3sm_diags/viewer/*.py | Updated sorting to handle time slices numerically |
| examples/ex9-snapshot-analysis/* | New example demonstrating snapshot analysis |
| docs/source/*.rst | Documentation updates for v3.1.0 features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| >>> validate_time_slice_format("5") | ||
| >>> validate_time_slice_format("42") | ||
| """ | ||
| pattern = r"^\d+$" |
Copilot
AI
Oct 21, 2025
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 docstring states that time slices 'must be non-negative integer indices', but the current pattern allows negative integers via the regex. Consider updating the pattern to explicitly reject negative values or clarifying the docstring.
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.
fyi @chengzhuzhang
tomvothecoder
left a comment
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.
My initial review. I am addressing my comments in an upcoming commit.
| >>> validate_time_slice_format("5") | ||
| >>> validate_time_slice_format("42") | ||
| """ | ||
| pattern = r"^\d+$" |
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.
fyi @chengzhuzhang
| | Native grid visualization (lat_lon_native) | Support for plotting data on native grids (e.g., cubed-sphere, unstructured grids) using UXarray, enabling visualization without regridding to preserve native grid features | Jill Zhang, Tom Vo | 3.1.0 | | ||
| | Snapshot analysis for core sets | Index-based time selection for snapshot analysis on core diagnostic sets (lat_lon, lat_lon_native, polar, zonal_mean_2d, meridional_mean_2d, zonal_mean_2d_stratosphere), allowing analysis of individual time steps instead of climatological means | Jill Zhang, Tom Vo | 3.1.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.
Thanks for including me!
- Refactor drivers for polar, meriodional_mean_2d, zonal_mean_2d, zonal_mean_xy to use `driver.utils.io._get_xarray_datasets()` - Add `driver.io._get_xarray_datasets()` utility to simplify fetching of xarray datasets based on time selection - Update references to `season` to `time_selection` with `TimeSelection` annotation - Move `_set_time_slice_name_yrs_attrs()` back to `LatLonNativeParameter` because it is only used there -- consider refactoring later - Remove `time_slice.py` as this functions were converted to `CoreParameter` methods
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.
Hi @chengzhuzhang, here's my initial refactor. Please review my changes in this commit f0e5426 (#1013). You can review my summary of changes below while referencing the related file(s) for each of the main topics.
- Results directory (successful): https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.zhang40/tests/1013-snapshot-analysis-core-sets/viewer/
- My next actions are to add unit tests and address any lingering items.
Summary of Changes
This refactor standardizes how time-sliced datasets are retrieved across multiple drivers, removes redundant I/O code, and improves consistency around time_selection handling.
New Utility
-
File:
driver/utils/io.py- Added
_get_xarray_datasets()to centralize logic for fetching xarray datasets based ontime_selection. - Simplifies driver implementations by providing a single entry point for time-sliced I/O.
- Added
Updated Dataset class
-
File:
driver/utils/dataset_xy.py- Added
get_time_sliced_dataset()which implementation logic focused on time sliced datasets, extracted fromget_climo_dataset(). - Removed time slice logic from
get_climo_dataset(), includingis_time_sliceargument.
- Added
Driver Refactors
-
Files:
driver/polar_driver.py,driver/meridional_mean_2d_driver.py,driver/zonal_mean_2d_driver.py,driver/zonal_mean_xy_driver.py- Updated these drivers to use
_get_xarray_datasets()instead of duplicating dataset-loading logic. - Ensures consistent behavior across drivers and reduces maintenance burden.
- Updated these drivers to use
-
Note:
driver/lat_lon_driver.pystill uses_get_ref_dataset()since it has a unique reference dataset workflow. This was left unchanged intentionally.
Parameter and API Updates
-
Files:
parameter/core_parameter.py,parameter/lat_lon_native_parameter.py- Added
_get_time_selection_to_use()to simplify process of determining the time selection type (time_slicesorseasons) and corresponding values. - Replaced
seasonwithtime_selectionand added theTimeSelectionannotation for clarity. - Moved
_set_time_slice_name_yrs_attrs()back intoLatLonNativeParameter, as it’s only used there -- added a note suggesting future refactoring or consolidation.
- Added
Code Cleanup
-
File Removed:
driver/utils/time_slice.py- All functionality migrated into
CoreParametermethods, eliminating redundancy and streamlining time-slice handling logic.
- All functionality migrated into
Overall Impact
- Unified I/O logic for time-sliced dataset retrieval.
- Reduced duplication across major drivers.
- Improved naming and parameter clarity (
time_selection). - Preserved
lat_lon_driver’s specialized reference dataset logic.
- Test coverage for get_time_sliced_dataset(), _add_time_series_filepath_attr(), check_values(), and get_xarray_datasets()`
tomvothecoder
left a comment
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.
Added unit tests in 38f05f9 (#1013). The test script ran successfully with all of my latest changes: https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.zhang40/tests/1013-snapshot-analysis-core-sets/viewer/. The GH Actions build is passing too.
I think this PR is ready for a final self-review and merge.
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.
Pull Request Overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
@tomvothecoder thank you for the improvement for the PR. I'm doing a full run test, and will merge if things look okay. |
|
Test results looks good. Merge. |
|
@chengzhuzhang Thanks! Great work on this PR. |
Description
Overview
The snapshot analysis was first implement for
lat_lon_nativeset for native model output. This PR implemented time slice support for 6 additional core diagnostic sets in e3sm_diags,extending the existing time slice analysis feature from lat_lon_native to other core sets.Diagnostic Sets Updated
Key Architecture Changes
Created new module with reusable functions:
Extended Dataset.get_climo_dataset() method:
Each driver now follows consistent pattern:
1. Get time selection parameters
time_slices = getattr(parameter, "time_slices", [])
has_seasons, has_time_slices = check_time_selection(seasons, time_slices, require_one=True)
2. Determine mode and data source
time_selections = time_slices if has_time_slices else seasons
is_time_slice_mode = has_time_slices
3. Load data conditionally
for time_selection in time_selections:
if is_time_slice_mode:
ds = dataset.get_climo_dataset(var, time_selection, is_time_slice=True)
parameter._set_name_yrs_attrs(dataset, time_selection) # After loading
else:
parameter._set_name_yrs_attrs(dataset, time_selection) # Before loading
ds = dataset.get_climo_dataset(var, time_selection)
Test Updates
signature
Benefits
Documentation with usage examples are added for v3.1.0
Checklist
If applicable: