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

Intake conversion Equatorial_thermal_and_zonal_velocity_structure #359

Merged
merged 10 commits into from
Sep 1, 2024

Conversation

rbeucher
Copy link
Contributor

@rbeucher rbeucher commented Jun 6, 2024

Following the discussion in issue #313, we propose converting the recipes to use Intake, given that the Cookbook is no longer supported and the ACCESS-NRI Intake catalog is now available.

A few months ago, @max-anu began working on this transition. This pull request contains the changes @max-anu made to the notebook specified in the title.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

review-notebook-app bot commented Aug 30, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-08-30T01:22:03Z
----------------------------------------------------------------

Line #6.        darray = catalog[expt].search(variable = 'temp', frequency = '1mon').to_dask(xarray_open_kwargs = {'use_cftime':True})

I think 'use_cftime':True is the default so you cam remove that.

This returns a dataset, so better to call it ds, or temp_ds


julia-neme commented on 2024-08-30T03:42:27Z
----------------------------------------------------------------

I don't think it's the default! Without that kwarg, I get the following warning:

/g/data/hh5/public/apps/miniconda3/envs/analysis3-24.04/lib/python3.10/site-packages/xarray/coding/times.py:987: SerializationWarning: Unable to decode time axis into full numpy.datetime64 objects, continuing using cftime.datetime objects instead, reason: dates out of range
  dtype = _decode_cf_datetime_dtype(data, units, calendar, self.use_cftime)

Copy link

review-notebook-app bot commented Aug 30, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-08-30T01:22:03Z
----------------------------------------------------------------

Line #7.        darray = darray['temp'].sel(time = exp_dict[ekey]['time_bounds']) - 273.15

I think its clearer to do the time selection and temperature change on seperate lines


Copy link

review-notebook-app bot commented Aug 30, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-08-30T01:22:04Z
----------------------------------------------------------------

Line #25.        darray = catalog[expt].search(variable = 'u', frequency = '1mon').to_dask(xarray_open_kwargs = {'use_cftime':True})

is it clearer to combine the two functions ?

i.e. :

ds_dict = catalog[expt].search(variable = ['temp','u'], frequency = '1mon').to_dataset_dict()

ds = xr.merge(ds_dict.values())



@anton-seaice
Copy link
Collaborator

I dropped some initial thoughts, I might struggle to get time in the next week for a proper review !

Copy link
Collaborator

I don't think it's the default! Without that kwarg, I get the following warning:

/g/data/hh5/public/apps/miniconda3/envs/analysis3-24.04/lib/python3.10/site-packages/xarray/coding/times.py:987: SerializationWarning: Unable to decode time axis into full numpy.datetime64 objects, continuing using cftime.datetime objects instead, reason: dates out of range
  dtype = _decode_cf_datetime_dtype(data, units, calendar, self.use_cftime)

View entire conversation on ReviewNB

Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

I tried to run this recipe but it fails at cell 12. I pushed the notebook with the error message.

@julia-neme
Copy link
Collaborator

Sorry, had a typo! Should run now.

Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

lgtm; thanks @julia-neme

@navidcy navidcy merged commit 84b3320 into main Sep 1, 2024
3 checks passed
@navidcy navidcy deleted the INTAKE_Equatorial_thermal_and_zonal_velocity_structure branch September 1, 2024 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants