xarray-contrib / xwrf

A lightweight interface for working with the Weather Research and Forecasting (WRF) model output in Xarray.
https://xwrf.readthedocs.io/
Apache License 2.0
56 stars 16 forks source link

Tutorial #72

Closed lpilz closed 2 years ago

lpilz commented 2 years ago

Change Summary

Tutorial showing xWRF usage.

Related issue number

Checklist

lpilz commented 2 years ago

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

andersy005 commented 2 years ago

@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 commented 2 years ago
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 commented 2 years ago

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 :)

kmpaul commented 2 years ago

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

Some comments:

  1. 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.)

  2. 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: https://github.com/pydata/xarray/issues/4024, https://github.com/pydata/xarray/pull/4036, https://github.com/pydata/xarray/issues/4161, and others, I'm sure.)

kmpaul commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

andersy005 commented 2 years ago

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

1) Safari

Screen Shot 2022-04-18 at 10 12 46 PM

2) Firefox

Screen Shot 2022-04-18 at 10 11 48 PM
lpilz commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

kmpaul commented 2 years ago

@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 commented 2 years ago

See #73

kmpaul commented 2 years ago

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

kmpaul commented 2 years ago

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 commented 2 years ago

thank you 🙏, @kmpaul! the fix looks great

kmpaul commented 2 years ago

Thanks, @andersy005! I'm appreciate that.

lpilz commented 2 years ago

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

kmpaul commented 2 years ago

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