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.

Sign up for GitHub

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

Address diffs v2.12.1 to v3 #907

Merged
merged 26 commits into from
Jan 15, 2025
Merged

Address diffs v2.12.1 to v3 #907

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a84b860
address_diffs_v2.12.1_to_v3
chengzhuzhang Dec 11, 2024
796ca4c
Revert "address_diffs_v2.12.1_to_v3"
chengzhuzhang Dec 11, 2024
0591c39
add_test_scripts
chengzhuzhang Dec 11, 2024
ee009c1
Fix incorrect logic for `udunits` conversions in `convert_units()`
tomvothecoder Dec 12, 2024
528bac6
Add complete run script
tomvothecoder Dec 16, 2024
8982c31
Update run script to save netCDF
tomvothecoder Dec 16, 2024
09e853c
Update dev dir for regression notebook
tomvothecoder Dec 16, 2024
b9da444
Add time slicing before `.load()` for performance
tomvothecoder Dec 17, 2024
4ad47a7
Add arm_diags and merra2 debug scripts
tomvothecoder Jan 7, 2025
b1a5a77
Fix hanging for derived vars with convert_units()
tomvothecoder Jan 8, 2025
3ea5422
Add arm_diags back into complete run script
tomvothecoder Jan 8, 2025
a76cfd8
Squeeze time dim for climo files before loading into memory
tomvothecoder Jan 9, 2025
af046a2
Update arm diags debug scripts with info on specific issues
tomvothecoder Jan 9, 2025
65f61ac
Add script debug H2OLNZ
tomvothecoder Jan 9, 2025
45277e6
Add H2OLNZ unit conversion from PR #874
tomvothecoder Jan 10, 2025
f346c51
Add v3 regression testing results
tomvothecoder Jan 10, 2025
94959cc
Add v2 data regression testing
tomvothecoder Jan 10, 2025
9221c5b
Update regression testing scripts
tomvothecoder Jan 10, 2025
313a641
update site data pre-processing script based on xarray
chengzhuzhang Jan 11, 2025
7020a25
Update v2 regression test notebooks
tomvothecoder Jan 14, 2025
d94ed40
Add `pywavelets` dependency to fix scipy `cwt` deprecation
tomvothecoder Jan 14, 2025
2144f51
Clean up postprocessing script for arm_diags
tomvothecoder Jan 14, 2025
60f53bc
figure out how to use pywavelets
tomvothecoder Jan 14, 2025
dd19a17
Add final v3 data regression test results
tomvothecoder Jan 15, 2025
01a91e4
Revert pywavelets code changes and constrain `scipy <1.15`
tomvothecoder Jan 15, 2025
1b0f998
Apply suggestions from code review
tomvothecoder Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions e3sm_diags/driver/qbo_driver.py
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

@chengzhuzhang We need to replace cwt from scipy with PyWavelets (#912) I'm trying to figure out where to pass the deg = 6 arg here. If you can try taking a look too that'd be great.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Click here for diff: https://github.com/E3SM-Project/e3sm_diags/pull/907/files/2144f51f07a9b855e1b763a3cc5f1a493bac7f68..60f53bc5165bb6bb61b549a3dee6d74627681856#diff-747eb4531216710f7645e357652e33c3fe74c293bbb311a1b9c298a8193f02af

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related SciPy docs:

Related PyWavelets docs:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whannah1 hey Walter, as we are preparing for the e3sm_diags v3 release, @tomvothecoder found that scipy.signal.cwt and scipy.signal.morlet2 are removed from latest scipy, and the suggestion was to use pywavelets instead. It seems that transition to use pywavelets API is not straightforward and not much documentation was found to help transition. I'm wondering if you are familiar with the this tool and could help re-write this _get_psd_from_wavelet function?

Copy link
Collaborator

@tomvothecoder tomvothecoder Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@chengzhuzhang I suggest that we:

  1. Not rush to finish this issue for v3.0.0rc1 due to high risk of side-effects and bugs with new, unfamiliar API
  2. Move this work to a separate PR and try to get it done during E3SM Unified RC testing phase
  3. Constrain scipy < 1.15 for now -- FYI @xylar for E3SM Unified (will update E3SM Confluence page)

Let me know thoughts.

UPDATE: Just saw Xylar's comment here related to 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All are reasonable! I was thinking along the same line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Constrain scipy < 1.15 for now -- FYI @xylar for E3SM Unified (will update E3SM Confluence page)

I want to make sure we're on the same page. This constraint would not be in E3SM-Unified, it would be in e3sm_diags. But it would be important that E3SM-Unified or its other dependencies don't contradict this constraint.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Constrain scipy < 1.15 for now -- FYI @xylar for E3SM Unified (will update E3SM Confluence page)

I want to make sure we're on the same page. This constraint would not be in E3SM-Unified, it would be in e3sm_diags. But it would be important that E3SM-Unified or its other dependencies don't contradict this constraint.

Yup we're on the same page. Thanks for confirming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomvothecoder, see comment from @whannah1 here. We can either implement this as-is, and testing and fine tune (if needed) during testing period. Or pinning scipy, and update to use pywavelets during testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@chengzhuzhang I'm going to pin scipy here and address #912 in a separate PR.

Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
from typing import TYPE_CHECKING, Dict, Literal, Tuple, TypedDict

import numpy as np
import pywt
import scipy.fftpack
import xarray as xr
import xcdat as xc
from pywt import cwt
from scipy.signal import detrend

from e3sm_diags.driver.utils.dataset_xr import Dataset
Expand Down Expand Up @@ -410,13 +410,23 @@ def _get_psd_from_wavelet(data: np.ndarray) -> Tuple[np.ndarray, np.ndarray]:
-------
Tuple(np.ndarray, np.ndarray)
The period and PSD arrays.

Notes
-----
- https://pywavelets.readthedocs.io/en/latest/ref/cwt.html#complex-morlet-wavelets
"""
deg = 6
period = np.arange(1, 55 + 1)
freq = 1 / period

# FIXME: Where do we pass in the `deg` argument for Omega0?
# scipy.signal.mortlet2() has a `deg` argument, but not
# pywt.ContinuousWavelet().
# Complex Morlet Wavelets ("cmorB-C")
wavelet_obj = pywt.ContinuousWavelet("cmorB-C", w=deg)
widths = deg / (2 * np.pi * freq)
cwtmatr = cwt(data, scipy.signal.morlet2, widths=widths, w=deg)

cwtmatr, _ = pywt.cwt(data, widths, wavelet_obj)
psd = np.mean(np.square(np.abs(cwtmatr)), axis=1)

return (period, psd)
Loading