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

Update global_mean_timeseries.py #352

Merged
merged 29 commits into from
Feb 7, 2025

Conversation

justin-richling
Copy link
Collaborator

Currently the time series plot is crashing when trying to check dimensions of the zm data. This is because the data was averaged before checking dims with pf.zm_validate_dims, which will fail because the 'lat' dim will no longer be present after spatial average. This PR will change order of checking zm dims first then it will average the data if it passes the dim check.

Also update LENS2 file grab since the files in the directory are not named exactly the same so add wildcard for searching.

Change order of checking zm dims

Also update LENS2 file grab since the files in the directory are not named exactly the same so add wildcard for searching
@brianpm
Copy link
Collaborator

brianpm commented Jan 11, 2025

I think this line:
has_lat_ref, has_lev_ref = pf.zm_validate_dims(ref_ts_da)
could still be problematic if zm_validate_dims returns None.

We can either fix zm_validate_dims (return (None, None), for example), or we might just change this to:

valdims = pf.zm_validate_dims(ref_ts_da)
if valdims is not None:
  has_lat_ref, has_lev_ref = valdims
else:
  has_lat_ref, has_lev_ref = False, False # I *think* this is what we'd expect here ... double check

@justin-richling
Copy link
Collaborator Author

justin-richling commented Jan 14, 2025

I think your'e right, if there is a scenario where there is not a lat dim, this would be a problem. I guess either change is sufficient because we want to shut it down if it's missing lat dim. @nusbaume any thoughts on which would be preferable? I can't seem to see an advantage of one over the other.

Either way, we should also probably put a check for when has_lat is None or False with a continue call too so that it doesn't try to get to ref_ts_da_ga = pf.spatial_average(ref_ts_da, weights=None, spatial_dims=None), obviously for the test case loop too.

@nusbaume
Copy link
Collaborator

@justin-richling If other scripts that are calling zm_validate_dims are checking for just a single None then I would vote to implement @brianpm's option. However, if none of them are doing this check then I would probably vote to modify zm_validate_dims itself to just return False for both variables in this case, as that should keep the interface the same everywhere while still protecting us from the situation where the latitude dimension or input data is invalid.

Anyways, that's my two cents. Hope that helps!

@justin-richling justin-richling marked this pull request as draft January 31, 2025 17:18
@justin-richling justin-richling marked this pull request as ready for review February 4, 2025 15:33
Copy link
Collaborator Author

@justin-richling justin-richling left a comment

Choose a reason for hiding this comment

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

All test cases work as intended

@justin-richling justin-richling merged commit 534e806 into NCAR:main Feb 7, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants