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

Update regridding notebook for v0.7.0 #646

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

chengzhuzhang
Copy link
Collaborator

@chengzhuzhang chengzhuzhang commented Apr 17, 2024

Description

For addressing: #589 (comment) but focus only on horizontal and vertical regridding notebooks

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

@chengzhuzhang
Copy link
Collaborator Author

chengzhuzhang commented Apr 18, 2024

Only a few minor changes that makes note books to run. Some changes noted:

  1. notebook setup: I had to include python -m ipykernel install --user --name xcdat_notebook --display-name xc070 to have the xcdat_notebook kernel to show up in Jupyter. Though I don't see this step in other example notebook.
  2. I added gsw-xarray package as a dependency which is needed by the vertical regridding notebook.
  3. In https://github.com/xCDAT/xcdat/blob/9193509919fe5123f989e2211d2517c266cbe097/docs/examples/regridding-horizontal.ipynb:
    Ln 8: created a new warning:/Users/zhang40/mambaforge/envs/xcdat_notebook/lib/python3.12/site-packages/xcdat/regridder/regrid2.py:195: RuntimeWarning: invalid value encountered in divide np.divide(, which I think is resulted by a recent change from @jasonb5, and the result figure seems to be deviated from the results on main (here) based my assessment with naked eyes. Not sure if this is expected.

@pochedls
Copy link
Collaborator

I don't mind including notebook setup instructions, but I had included the following text in order to avoid repeating the instructions across all notebooks:

Users can install their own instance of xcdat and follow these examples using their own environment (e.g., with vscode, Jupyter, Spyder, iPython) or enable xcdat with existing JupyterHub instances. The conda environment used in this notebook includes xcdat, xesmf, matplotlib, ipython, ipykernel, cartopy, and jupyter:

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Apr 18, 2024

Only a few minor changes that makes note books to run. Some changes noted:

1. notebook setup: I had to include `python -m ipykernel install --user --name xcdat_notebook --display-name xc070` to have the xcdat_notebook kernel to show up in Jupyter. Though I don't see this step in other example notebook.

I was able to use the Python kernel from the xcdat_notebook environment in Jupyter. This kernel is listed as "Python 3 (ipykernel)", not xcdat_notebook. I don't think it is necessary to call the python -m ipykernel command for this case.
The import xcdat line works fine in the screenshot below:

image

Commands used:

$ conda create -n xcdat_notebook -c conda-forge xcdat xesmf matplotlib ipython ipykernel cartopy nc-time-axis jupyter
$ conda activate xcdat_notebook
$ jupyter notebook

@chengzhuzhang
Copy link
Collaborator Author

chengzhuzhang commented Apr 18, 2024

@tomvothecoder I did not know Python3 (ipykernel) is the one with xcdat. I thought it is just bare-bone python3. I agree python -m ipykernel is not necessary, but when multiple kernels are with the same name, it can cause some confusion. And I'm still fine with registering the xcdat_note book with a specified name at this point.

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Apr 18, 2024

@tomvothecoder I did not know Python3 (ipykernel) is the one with xcdat. I thought it is just bare-bone python3. I agree python -m ipykernel is not necessary, but when multiple kernels are with the same name, it can cause some confusion. And I'm still fine with registering the xcdat_note book with a specified name at this point.

If Jupyter is invoked from within a conda environment (xcdat_notebook in this case), the Python kernel of that environment is defaulted as "Python 3 (ipykernel)". I don't think there should be any other kernels listed unless the user manually adds them (e.g., python -m ipykernel), since the Jupyter instance is isolated within the conda env (not 100% sure though).

On the other hand, public Jupyter instances would probably have multiple kernels listed (e.g., NERSC Jupyter Hub) since kernels are manually registered to that public instance.

We can either 1) add a note on the notebooks to let the the user know Python3 (ipykernel) is the kernel of xcdat_notebook by default or 2) add the extra step to register the kernel in all of the notebooks, which is necessary if the tries to use the notebooks through a Jupyter instance invoked elsewhere and not xcdat_notebook.

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Apr 18, 2024

I don't think there should be any other kernels listed unless the user manually adds them (e.g., python -m ipykernel), since the Jupyter instance is isolated within the conda env (not 100% sure though).

I just confirmed that no other kernels are registered in the Jupyter instance invoked by xcdat_notebook. The only one listed is "Python 3 (ipykernel)", which is the ipykernel of xcdat_notebook.

image

@chengzhuzhang
Copy link
Collaborator Author

chengzhuzhang commented Apr 18, 2024

This is what I see from my end...In my Jupyter instance, it includes even my old conda env registered with Jupyter.
Screen Shot 2024-04-18 at 10 35 34 AM

@chengzhuzhang
Copy link
Collaborator Author

We can either 1) add a note on the notebooks to let the the user know Python3 (ipykernel) is the kernel of xcdat_notebook by default or 2) add the extra step to register the kernel in all of the notebooks, which is necessary if the tries to use the notebooks through a Jupyter instance invoked elsewhere and not xcdat_notebook.

I think it is a good solution!

@tomvothecoder
Copy link
Collaborator

This is what I see from my end...In my Jupyter instance, it includes even my old conda env registered with Jupyter.

Thanks for the screenshot! Yeah I'm not sure why those kernels are registered with the Jupyter instance invoked through xcdat_notebook. Maybe that's how it's supposed to be.

In any case, I will update all the notebooks using both solutions above in a separate PR.

@chengzhuzhang
Copy link
Collaborator Author

This is what I see from my end...In my Jupyter instance, it includes even my old conda env registered with Jupyter.

Thanks for the screenshot! Yeah I'm not sure why those kernels are registered with the Jupyter instance invoked through xcdat_notebook. Maybe that's how it's supposed to be.

In any case, I will update all the notebooks using both solutions above in a separate PR.

Thank you for implementing it!

@jasonb5
Copy link
Collaborator

jasonb5 commented Apr 18, 2024

@chengzhuzhang @tomvothecoder
The warning /Users/zhang40/mambaforge/envs/xcdat_notebook/lib/python3.12/site-packages/xcdat/regridder/regrid2.py:195: RuntimeWarning: invalid value encountered in divide np.divide( is fine, it occurs when regridding with a mask and the sum of the input weights for a cell is zero, which is just missing data in that cell.

I do see the visual difference in the last example, this would be expected as I reworked how masking worked. The updated result should be more accurate, it appears to be visually.

@chengzhuzhang
Copy link
Collaborator Author

@chengzhuzhang @tomvothecoder The warning /Users/zhang40/mambaforge/envs/xcdat_notebook/lib/python3.12/site-packages/xcdat/regridder/regrid2.py:195: RuntimeWarning: invalid value encountered in divide np.divide( is fine, it occurs when regridding with a mask and the sum of the input weights for a cell is zero, which is just missing data in that cell.

I do see the visual difference in the last example, this would be expected as I reworked how masking worked. The updated result should be more accurate, it appears to be visually.

Got it. Thank you for the calcification @jasonb5

@tomvothecoder
Copy link
Collaborator

@chengzhuzhang @tomvothecoder The warning /Users/zhang40/mambaforge/envs/xcdat_notebook/lib/python3.12/site-packages/xcdat/regridder/regrid2.py:195: RuntimeWarning: invalid value encountered in divide np.divide( is fine, it occurs when regridding with a mask and the sum of the input weights for a cell is zero, which is just missing data in that cell.

I do see the visual difference in the last example, this would be expected as I reworked how masking worked. The updated result should be more accurate, it appears to be visually.

Great! @chengzhuzhang You can merge this PR if you think it is ready.

@chengzhuzhang chengzhuzhang force-pushed the regriding_notebooks_update_0.7.0 branch from 9193509 to 5271b73 Compare April 22, 2024 17:02
@chengzhuzhang chengzhuzhang merged commit 0214694 into main Apr 22, 2024
5 checks passed
@chengzhuzhang chengzhuzhang deleted the regriding_notebooks_update_0.7.0 branch April 22, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants