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

[doc] analysis tutorial #249

Merged
merged 9 commits into from Nov 4, 2021
Merged

Conversation

nutrik
Copy link
Collaborator

@nutrik nutrik commented Oct 31, 2021

Hi Dion,
I don't know why but Actions on my account don't currently work (trying to get in touch with GitHub support). Could you please review my commit for docs? I will probably do development in team-ocean next time (not sure it is going to help though 🤨) Sorry!

There are several hidden changes in attributes of coordinates. They are done to eliminate some problems related to textwrap module, which cannot handle the default attribute names from netCDF files 🤷‍♂️

@dionhaefner
Copy link
Collaborator

Maybe it could pay off to delete your fork and re-create it? Since it's from a time before GitHub Actions existed that might mess with it.

I'll review this as soon as I can.

Copy link
Collaborator

@dionhaefner dionhaefner left a comment

Choose a reason for hiding this comment

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

I think this is a nice range of examples. If you have more time to work on this I think it could be nice to have more explanations between the code blocks. Otherwise we can fix the issues and merge for now.

.github/workflows/build-docs.yml Outdated Show resolved Hide resolved
doc/index.rst Outdated Show resolved Hide resolved
doc/tutorial/analysis.rst Outdated Show resolved Hide resolved
doc/tutorial/analysis.rst Outdated Show resolved Hide resolved
Comment on lines 44 to 59
.. ipython:: python
:suppress:

ds.xt.attrs["long_name"] = "longitude"
ds.xt.attrs["units"] = "deg"
ds.yt.attrs["long_name"] = "latitude"
ds.yt.attrs["units"] = "deg"
ds.zt.attrs["long_name"] = "depth"
ds.zt.attrs["units"] = "m"

.. ipython:: python

ds.xu.attrs["long_name"] = "longitude"
ds.xu.attrs["units"] = "deg"
ds.yu.attrs["long_name"] = "latitude"
ds.yu.attrs["units"] = "deg"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you do this? It works fine for me if I remove this. OK I see the problem. You used nco on the output files which injected some strings that are messing up the parsing. This looks like a bug in h5netcdf to me. I will investigate this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened an issue here: h5netcdf/h5netcdf#116

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use the netcdf4 engine for this tutorial instead. All you should need to do is add netcdf4 to the requirements (it's used by default in xarray). Then you don't need to touch the attributes, and you can decode times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will do.
However, I would keep decode_times=False, since the decoded time has unit of ns, which is kind of pointless for time span of a year.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are many ways to skin a cat, and your way definitely works, I just feel that since this is an "official" tutorial we should promote best practices when working with xarray. So I changed it to what is recommended in the xarray docs.

doc/tutorial/analysis.rst Outdated Show resolved Hide resolved
doc/tutorial/analysis.rst Outdated Show resolved Hide resolved
doc/tutorial/analysis.rst Outdated Show resolved Hide resolved
doc/tutorial/analysis.rst Outdated Show resolved Hide resolved
doc/tutorial/analysis.rst Outdated Show resolved Hide resolved
nutrik and others added 6 commits November 2, 2021 13:27
change intro text

Co-authored-by: Dion Häfner <mail@dionhaefner.de>
wip

Co-authored-by: Dion Häfner <mail@dionhaefner.de>
wip

Co-authored-by: Dion Häfner <mail@dionhaefner.de>
wip

Co-authored-by: Dion Häfner <mail@dionhaefner.de>
change path to OUTPUT_FILES

Co-authored-by: Dion Häfner <mail@dionhaefner.de>
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #249 (f3be41b) into master (21fcd3a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #249   +/-   ##
=======================================
  Coverage   82.34%   82.34%           
=======================================
  Files          84       84           
  Lines        7257     7257           
  Branches      875      875           
=======================================
  Hits         5976     5976           
  Misses        984      984           
  Partials      297      297           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21fcd3a...f3be41b. Read the comment docs.

@dionhaefner
Copy link
Collaborator

What do you think of this? I applied some xarray best practices, switched the colormaps to cmocean, and made it a bit more reproducible by describing how to get the data.

@nutrik
Copy link
Collaborator Author

nutrik commented Nov 4, 2021

It looks great! Thank you!

@dionhaefner dionhaefner merged commit 7fe8ccf into team-ocean:master Nov 4, 2021
@dionhaefner
Copy link
Collaborator

And we're live. Thanks!

@nutrik nutrik deleted the analysis_tutorial branch September 6, 2022 08:58
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.

None yet

2 participants