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

Model agnostic tutorial #226

Merged
merged 11 commits into from
Jan 26, 2023
Merged

Model agnostic tutorial #226

merged 11 commits into from
Jan 26, 2023

Conversation

aidanheerdegen
Copy link
Contributor

Model agnostic tutorial using x-array and pint closes #211

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@angus-g angus-g left a comment

Choose a reason for hiding this comment

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

Don't need explorer tools changes? Plus we can be a tidier with operations rather than going all the way through dimension names.

@aidanheerdegen
Copy link
Contributor Author

Don't need explorer tools changes? Plus we can be a tidier with operations rather than going all the way through dimension names.

I don't think I understand.

@angus-g
Copy link
Contributor

angus-g commented Jan 24, 2023

Just that you've also modified the explorer tools notebook in this PR! Not sure if that was intentional?

@aidanheerdegen
Copy link
Contributor Author

Just that you've also modified the explorer tools notebook in this PR! Not sure if that was intentional?

Ah no. I did see that, but just assumed it was the change from the other PR. Will fix.

Plus we can be a tidier with operations rather than going all the way through dimension names.

In this notebook? If so, I don't understand.

@angus-g
Copy link
Contributor

angus-g commented Jan 24, 2023

In this notebook? If so, I don't understand.

Was referring to the cf accessor, couldn't think of what to call it! You got what I meant anyway 😁 All good

@aidanheerdegen
Copy link
Contributor Author

You got what I meant anyway 😁 All good

Ahh .. yeah, once you pointed it out I took a look, very neat!

Do you know off hand the limits of the method wrapping/translation? I think it is probably noting that it is worth using, but maybe won't work for more exotic functions .. or maybe it does work with everything?

@angus-g
Copy link
Contributor

angus-g commented Jan 24, 2023

Do you know off hand the limits of the method wrapping/translation?

Not sure how it works actually, I think the intention is for it to work everywhere in xarray at least, and if something doesn't work it's a bug on their end. I'd say it's a safe bet to drop in to any method requiring a dimension argument.

@aidanheerdegen
Copy link
Contributor Author

I initially tried also with some ice data, but the multiple grids are a bit annoying. I've realised I can fix that pretty easily, so I think I'll add an ice example as well, just to show it is possible and quite flexible.

@navidcy
Copy link
Collaborator

navidcy commented Jan 24, 2023

okie! some sea ice vars would be great to have here also!
happy to have a look at the notebook! @aidanheerdegen ping me when you are happy for me to do that.

@edoddridge
Copy link
Collaborator

This looks great! Can't wait to see the ice addition.

@aidanheerdegen
Copy link
Contributor Author

Okey done. I've added an ice example. It is ugly because the coordinates etc in the ice files are terrible. I don't really like the "now do some barely explained magic" but this isn't a tutorial about fixing ice coordinate data so I haven't dwelt on it. Didn't want to take away from the message.

I kept all the errors in to hopefully show how problems like that can be understood and overcome. Hope you like it @edoddridge

@navidcy
Copy link
Collaborator

navidcy commented Jan 25, 2023

The coordinates in ice files are terrible. I am voicing this also. I'm glad somebody finally had the courage to admit this in public!

@angus-g
Copy link
Contributor

angus-g commented Jan 25, 2023

That ice business seems like a bug, cf_xarray only forgets about the specific coordinates after the mean:

In [21]: d.Tair_m.cf
Out[21]: 
Coordinates:
- CF Axes:   X, Y, Z, T: n/a

- CF Coordinates:   longitude: ['TLON']
                    latitude: ['TLAT']
                    vertical, time: n/a

- Cell Measures:   area, volume: n/a

- Standard Names:   n/a

- Bounds:   n/a

In [22]: d.Tair_m.mean("time").cf
Out[22]: 
Coordinates:
- CF Axes:   X, Y, Z, T: n/a

- CF Coordinates:   longitude: ['TLON', 'ULON']
                    latitude: ['TLAT', 'ULAT']
                    vertical, time: n/a

- Cell Measures:   area, volume: n/a

- Standard Names:   n/a

- Bounds:   n/a

I've raised this as a bug xarray-contrib/cf-xarray#396

@aidanheerdegen
Copy link
Contributor Author

That ice business seems like a bug, cf_xarray only forgets about the specific coordinates after the mean

Nice pick-up Angus. I'll change the wording.

@dcherian
Copy link

dcherian commented Jan 25, 2023

Hi 👋

I came over from the cf-xarray bug report and just wanted to say this is very cool to see.

When this notebook goes live, we'd appreciate a PR linking to it from our docs.

Our current ocean model demo really emphasizes the vertical aspect, so this would be a nice complement

@navidcy
Copy link
Collaborator

navidcy commented Jan 25, 2023

Hi @dcherian! That’s great to hear! :)

@review-notebook-app
Copy link

review-notebook-app bot commented Jan 25, 2023

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2023-01-25T21:06:22Z
----------------------------------------------------------------

SST_time_mean.cf.plot(ax=ax,
                      x='longitude', y='latitude', 
                      transform=ccrs.PlateCarree(),
                      vmin=-2, vmax=30, extend='both',
                      cmap=cm.cm.thermal)

aidanheerdegen commented on 2023-01-26T01:57:17Z
----------------------------------------------------------------

This is to align indentation?

@review-notebook-app
Copy link

review-notebook-app bot commented Jan 25, 2023

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2023-01-25T21:06:23Z
----------------------------------------------------------------

    da_time_mean.cf.plot(ax=ax,
                         x='longitude', y='latitude', 
                         transform=ccrs.PlateCarree(),
                         vmin=-2, vmax=30, extend='both',
                         cmap=cm.cm.thermal)


aidanheerdegen commented on 2023-01-26T01:57:30Z
----------------------------------------------------------------

And this too I guess?

@review-notebook-app
Copy link

review-notebook-app bot commented Jan 25, 2023

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2023-01-25T21:06:23Z
----------------------------------------------------------------

Line #1.    ice_air_temp = ice_air_temp.drop_vars(['ULON', 'ULAT'])


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 25, 2023

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2023-01-25T21:06:24Z
----------------------------------------------------------------

Line #1.    ice_grid = xr.open_dataset('/g/data/ik11/inputs/access-om2/input_eee21b65/cice_025deg/grid.nc').rename({'ny': 'nj', 'nx': 'ni'})


@review-notebook-app
Copy link

review-notebook-app bot commented Jan 25, 2023

View / edit / reply to this conversation on ReviewNB

navidcy commented on 2023-01-25T21:06:25Z
----------------------------------------------------------------

    da_time_mean.cf.plot(ax=ax,
                         x='longitude', y='latitude', 
                         transform=ccrs.PlateCarree(),
                         vmin=vmin, vmax=vmax, extend='both',
                         cmap=cm.cm.thermal)

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.

@aidanheerdegen this is great! Two comments, don't feel that you need to act on them:

  • The notebook demonstrates also things that won't work so some cells fail on purpose. This, I presume, will make this tutorial not a good candidate for the automatic testing workflow.

  • I'm not sure if defining functions that plot is a good strategy as they are not so re-usable. I generally prefer to define a function that will do the conversions/transformations on the data array and then have another one that plots. But I feel that's a personal preference...

@aidanheerdegen
Copy link
Contributor Author

Hi @dcherian,

Thanks for stopping by, but even more thanks for the great work you've done developing this and other tools. Much appreciated.

When this notebook goes live, we'd appreciate a PR linking to it from our docs.

Our current ocean model demo really emphasizes the vertical aspect, so this would be a nice complement

Sure thing. I'll make a PR to add it to your examples.

TBH I'd been telling people to use cf-xarray for ages, but hadn't had an opportunity to use it properly until now, and it is even more transformative than I had realised. It is a great tool.

@navidcy
Copy link
Collaborator

navidcy commented Jan 26, 2023

I admit, I've been hearing @aidanheerdegen saying this for ages but when I saw the tutorial it was when I really listened.

Copy link
Contributor Author

This is to align indentation?


View entire conversation on ReviewNB

Copy link
Contributor Author

And this too I guess?


View entire conversation on ReviewNB

@aidanheerdegen
Copy link
Contributor Author

Ok, fixed all those formatting suggestions, and found a couple of other typos. I don't have merge permissions @navidcy. Can you do it. Squash the whole thing into a single commit.

@dcherian
Copy link

Thanks for the kind words!

Please open issues for any features you might find useful.

@navidcy navidcy self-requested a review January 26, 2023 04:57
@navidcy
Copy link
Collaborator

navidcy commented Jan 26, 2023

And this too I guess?

View entire conversation on ReviewNB

Yeap, it was to align the indentation so my OCD eyes are not triggered.

@navidcy
Copy link
Collaborator

navidcy commented Jan 26, 2023

@angus-g the PR needs your approval? are you happy?

@aidanheerdegen
Copy link
Contributor Author

@angus-g the PR needs your approval? are you happy?

I'm pretty sure he was ok with it, should be fine to merge

#226 (comment)

@navidcy navidcy merged commit b855641 into master Jan 26, 2023
@navidcy navidcy deleted the aidanheerdegen/issue-211 branch January 26, 2023 05:55
@navidcy
Copy link
Collaborator

navidcy commented Jan 26, 2023

Done! :) Open the PR at cf-xarray?

@aidanheerdegen
Copy link
Contributor Author

Done! :) Open the PR at cf-xarray?

In hand

@access-hive-bot
Copy link

This pull request has been mentioned on ACCESS Hive Community Forum. There might be relevant details there:

https://forum.access-hive.org.au/t/cosima-hackathon-v2-0-tuesday-january-24th-2023/307/46

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.

New Tutorial: how to use cf-xarray
6 participants