Tutorial #72

lpilz · 2022-04-13T10:40:40Z

Change Summary

Tutorial showing xWRF usage.

Related issue number

Towards Tutorial on xWRF #69

Checklist

Unit tests for the changes exist
Tests pass on CI
Documentation reflects the changes where applicable

lpilz · 2022-04-13T15:07:22Z

@andersy005 Can you advise me on how to properly integrate the jupyter notebooks into the docs?

andersy005 · 2022-04-13T15:41:39Z

@andersy005 Can you advise me on how to properly integrate the jupyter notebooks into the docs?

@lpilz, since we are already using markdown for docs, i would recommend using myst + jupytext combo to author the notebooks as well. here's an example of a notebook in a myst markdown format: https://raw.githubusercontent.com/intake/intake-esm/main/docs/source/tutorials/index.md. with myst + jupytext, the git diff integration is way smoother than the native notebook's (which is a huge plus in my opinion)

here's a pointer to the docs as well: https://myst-nb.readthedocs.io/en/latest/use/markdown.html

feel free to ping me whenever you need any feedback and/or clarification

dcherian · 2022-04-13T20:08:07Z

Exception occurred:
  File "/home/docs/checkouts/readthedocs.org/user_builds/xwrf/conda/72/lib/python3.9/site-packages/jupyter_client/kernelspec.py", line 294, in get_kernel_spec
    raise NoSuchKernel(kernel_name)
jupyter_client.kernelspec.NoSuchKernel: No such kernel named xwrf-dev

I've had trouble with writing a notebook using nb_conda_kernels making the RTD build fail. If you resave the notebook with just "Python3 (ipykernel)" (i.e.choose the base kernel because the RTD build will need to use that after activating the environment), the build should move forward.

lpilz · 2022-04-14T09:42:26Z

Alright, thanks @andersy005 and @dcherian for your help! After a bit of trial and error, everything seems to work now and this is ready for review from my end. Looking forward to your suggestions :)

docs/source/tutorials/Overview.md

docs/source/tutorials/metpy.md

kmpaul · 2022-04-14T19:45:31Z

@lpilz: This is looking great! Thanks for doing this!

Some comments:

I'm wondering if we should reference the non-code name of the project as simply xWRF, instead of xWRF (i.e., without the back-ticks). My thinking here is that back-ticks should be used for the name of actual code. For example, the package that you import would be xwrf, but the "English name" of the package is xWRF. (Does that make sense? I'm open to other suggestions.)
It appears that the xarray HTML reprs do not play well with dark mode iin the furo Sphinx theme. Some of the text is rendering with too little contrast to visually see in dark mode, but it displays beautifully with light mode. I don't think this a problem for xWRF to solve, but it is something that needs further investigation and a new issue in the right place needs to be made (or a PR, if we can figure out how to fix it). (Note that there are lots of dark-mode display problems that have come up: small contrast of html view in VScode darkmode pydata/xarray#4024, support darkmode pydata/xarray#4036, Dark theme-friendly HTML Dataset and DataArray reprs for jupyter notebooks? pydata/xarray#4161, and others, I'm sure.)

dcherian · 2022-04-14T19:54:40Z

docs/source/tutorials/xgcm.md

+
+def interp_and_keep_attrs(grid, da, axis):
+    _attrs = da.attrs
+    da = grid.interp(da, axis=axis)


This makes it seem like the "stagger" attribute should not be present at all.

The information about staggering is already present in the dimension names, indeed that is the xarray+xgcm data model: we know where the variables are on the grid by looking at names of dimensions.

This is a point, which @jthielen also raised in #70. Maybe we should discuss this in a new issue? My argument would be that there might be code in use depending on these attributes, which would break if we stripped them.

I just read this one again and maybe we should refrain from manually adding the stagger attribute here, since this is really not needed. My thinking was just that somebody might want to simply copy and paste this code...

docs/source/tutorials/Overview.md

kmpaul · 2022-04-18T19:00:11Z

@lpilz: I believe I have developed a "hack" to fix the xarray HTML repr display issues. It turns out the that Xarray HTML repr includes/ships CSS that assumes that any theme changes will be contained in the html element's theme attribute. But the Sphinx Furo theme assumes that the body element will have a data-theme attribute containing the theme (dark or light or auto). So, I added some custom javascript to force the html[theme] attribute to match the body[data-theme] attribute.

kmpaul · 2022-04-18T19:01:01Z

I'm not sure what the proper fix for this is...whether it should go in the Furo theme or in Xarray to specifically deal with the Furo theme.

kmpaul · 2022-04-18T19:03:44Z

BTW, this fix works on Chrome, and I haven't verified with other browsers.

andersy005 · 2022-04-19T04:15:37Z

BTW, this fix works on Chrome, and I haven't verified with other browsers.

unfortunately, this is still not functional on non-chromium based browsers

Safari

Firefox

lpilz · 2022-04-19T09:28:47Z

@kmpaul

I'm wondering if we should reference the non-code name of the project as simply xWRF, instead of xWRF

I also put some thought into that and decided to preliminarily go with the backticked version simply because of capitalization issues at the start of sentences. However, I agree that it adds a level of highlight that is maybe too much. I'll change it and we can just see if we like it like that.

lpilz · 2022-04-19T11:10:04Z

@kmpaul Thanks for your work on the styling issue. Unfortuately, I don't have a lot of experience with front end engineering, so I won't be able to help a lot on this. But if there is something I can do nonetheless - don't hesitate to delegate.

andersy005 · 2022-04-19T13:35:22Z

In my opinion, we should document the styling issue in a separate issue and address it at a later time

kmpaul · 2022-04-19T15:12:05Z

@andersy005: I think that's fine. Yes. I realized that my fix might be browser limited...but I wasn't expecting it to be that limited. Since it displays fine with the light theme of Furo, which is always an option, let's not let the styling issue hold up this PR.

kmpaul · 2022-04-19T15:16:48Z

See #73

kmpaul · 2022-04-19T15:22:24Z

Incidentally, I implemented the "proper fix" for this in Xarray yesterday (see pydata/xarray#6500 and pydata/xarray#6501). So, after the next release of Xarray, we won't need this hacky fix anyway.

kmpaul · 2022-04-19T15:49:34Z

Well... So, I know we said we'd fix this in another PR, but it turns out the fix was really easy. It shouldn't have worked on Chrome, either, but it turns out Chrome is a little more forgiving about assigning values to const variables. So, I just fixed it.

andersy005 · 2022-04-19T16:48:19Z

thank you 🙏, @kmpaul! the fix looks great

kmpaul · 2022-04-19T16:49:02Z

Thanks, @andersy005! I'm appreciate that.

lpilz · 2022-04-19T17:33:59Z

I concur, nice work. Thanks a lot @kmpaul :)

kmpaul · 2022-04-19T17:34:54Z

@lpilz, you are welcome! Glad I could help!

lpilz added 4 commits April 13, 2022 12:09

Initial commit

34ea31c

Initial commit

daaf047

Updated Overview

7199c5f

Update

a64b4a6

lpilz added 7 commits April 14, 2022 10:51

Trying out MyST

333af3e

Changed Kernels

880ada2

Update

a850bc1

Next try :D

40b1991

Hopefully fixed index

e4611e9

Removed spaces from filenames

c708ffa

Small changes making sections more sensible

55fe5b8

lpilz marked this pull request as ready for review April 14, 2022 12:04

andersy005 reviewed Apr 14, 2022

View reviewed changes

docs/source/tutorials/Overview.md Outdated Show resolved Hide resolved

andersy005 reviewed Apr 14, 2022

View reviewed changes

docs/source/tutorials/metpy.md Show resolved Hide resolved

dcherian reviewed Apr 14, 2022

View reviewed changes

docs/source/tutorials/Overview.md Show resolved Hide resolved

Add custom JS to fix furo theme changes

27f847e

Implemented suggestions

fb32df9

Fix for Safari and Firefox

0e19d5f

This was referenced Apr 19, 2022

Furo theme Xarray styling is bad on non-chromium browsers #73

Closed

Remove dark-theme hack on next Xarray release #74

Open

Removed superfluous stagger attribute

b955f86

lpilz merged commit 62a9613 into xarray-contrib:main Apr 21, 2022

lpilz deleted the tutorial branch April 21, 2022 12:35

jthielen mentioned this pull request Jul 13, 2022

First Release? #51

Closed

3 tasks

Tutorial #72

Tutorial #72

Uh oh!

Conversation

lpilz commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Related issue number

Checklist

Uh oh!

lpilz commented Apr 13, 2022

Uh oh!

andersy005 commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcherian commented Apr 13, 2022

Uh oh!

lpilz commented Apr 14, 2022

Uh oh!

Uh oh!

Uh oh!

kmpaul commented Apr 14, 2022

Uh oh!

dcherian Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

lpilz Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

lpilz Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kmpaul commented Apr 18, 2022

Uh oh!

kmpaul commented Apr 18, 2022

Uh oh!

kmpaul commented Apr 18, 2022

Uh oh!

andersy005 commented Apr 19, 2022

Uh oh!

lpilz commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpilz commented Apr 19, 2022

Uh oh!

andersy005 commented Apr 19, 2022

Uh oh!

kmpaul commented Apr 19, 2022

Uh oh!

kmpaul commented Apr 19, 2022

Uh oh!

kmpaul commented Apr 19, 2022

Uh oh!

kmpaul commented Apr 19, 2022

Uh oh!

andersy005 commented Apr 19, 2022

Uh oh!

kmpaul commented Apr 19, 2022

Uh oh!

lpilz commented Apr 19, 2022

Uh oh!

kmpaul commented Apr 19, 2022

Uh oh!

Uh oh!

lpilz commented Apr 13, 2022 •

edited

Loading

andersy005 commented Apr 13, 2022 •

edited

Loading

lpilz Apr 20, 2022 •

edited

Loading

lpilz commented Apr 19, 2022 •

edited

Loading