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

Intake conversion Regridding example; take #2 #413

Merged
merged 16 commits into from
Jul 22, 2024
Merged

Conversation

navidcy
Copy link
Collaborator

@navidcy navidcy commented Jul 7, 2024

I tried to work with #366 but I couldn't resolve the merge conflicts...
So I opened this PR to convert the Regridding example to use Intake instead of cookbook.

Also this PR deals with #418 regarding the Regridding example.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@navidcy navidcy added the 🛸 updating An existing notebook needs to be updated label Jul 7, 2024
@navidcy navidcy requested a review from a team July 7, 2024 09:48
@navidcy
Copy link
Collaborator Author

navidcy commented Jul 7, 2024

Took the opportunity to rename/retitle the notebook as per discussion in #412

Copy link

review-notebook-app bot commented Jul 8, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-08T07:05:56Z
----------------------------------------------------------------

Line #1.    import pandas as pd

This isn't used


Copy link

review-notebook-app bot commented Jul 8, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-08T07:05:56Z
----------------------------------------------------------------

Line #5.    ssh_1 = ds['sea_level'].sel(time=slice('2000-01-01', '2001-12-31')).cf.chunk({'time': 'auto', 'longitude': -1, 'latitude': -1})

I suggest we just .load() rather than rechunk ...


Copy link

review-notebook-app bot commented Jul 8, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-08T07:05:57Z
----------------------------------------------------------------

Line #5.    ssh_025 = ds['sea_level'].sel(time=slice('2000-01-01', '2001-12-31')).cf.chunk({'time': 'auto', 'longitude': -1, 'latitude': -1})

I suggest we just .load() rather than rechunk ...


navidcy commented on 2024-07-09T12:19:12Z
----------------------------------------------------------------

I left the chunck as it might be needed if you load a larger data array that you can't .load().What do you think?

anton-seaice commented on 2024-07-10T00:27:59Z
----------------------------------------------------------------

It doesn't appear to have made any difference to the times (or perhaps its slower?) so without .load is fine

navidcy commented on 2024-07-11T14:20:22Z
----------------------------------------------------------------

OK, done

Copy link

review-notebook-app bot commented Jul 8, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-08T07:05:58Z
----------------------------------------------------------------

Line #5.    ssh_010 = ds['sea_level'].sel(time=slice('2000-01-01', '2001-12-31')).cf.chunk({'time': 'auto', 'longitude': -1, 'latitude': -1})

I suggest we just .load() rather than rechunk ...


Copy link

review-notebook-app bot commented Jul 8, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-08T07:05:58Z
----------------------------------------------------------------

Line #4.    regridder_1degACCESSOM2_1deg = xesmf.Regridder(ds_in_1deg, ds_out, 'bilinear', periodic=True,

Can you run black or something on this notebook? The formatting is kind of messy


navidcy commented on 2024-07-09T12:19:34Z
----------------------------------------------------------------

I don't know how to do that! Can you do it perhaps?

navidcy commented on 2024-07-09T12:20:00Z
----------------------------------------------------------------

I know how to run black on .py scripts... Is it the same but black notebook.ipynb?

anton-seaice commented on 2024-07-10T00:27:17Z
----------------------------------------------------------------

Yes correct - its in conda/analysis3 so you should be able to run that in the ARE terminal or a Gadi login

navidcy commented on 2024-07-11T14:20:55Z
----------------------------------------------------------------

hm... I can't... can we leave this for some other time?

@anton-seaice
Copy link
Collaborator

I think this notebook might not need dask at all actually.

@anton-seaice
Copy link
Collaborator

I think this notebook might not need dask at all actually.

Nevermind .. it does need dask.

@navidcy
Copy link
Collaborator Author

navidcy commented Jul 9, 2024

@anton-seaice, thanks for the comments!I think I addressed all of them!

@navidcy
Copy link
Collaborator Author

navidcy commented Jul 9, 2024

I think this notebook might not need dask at all actually.

But what if you load more than one snapshot of fields you wanna regrid?

Copy link
Collaborator Author

navidcy commented Jul 9, 2024

I left the chunck as it might be needed if you load a larger data array that you can't .load().What do you think?


View entire conversation on ReviewNB

Copy link
Collaborator Author

navidcy commented Jul 9, 2024

I don't know how to do that! Can you do it perhaps?


View entire conversation on ReviewNB

Copy link
Collaborator Author

navidcy commented Jul 9, 2024

I know how to run black on .py scripts... Is it the same but black notebook.ipynb?


View entire conversation on ReviewNB

Copy link
Collaborator

Yes correct - its in conda/analysis3 so you should be able to run that in the ARE terminal or a Gadi login


View entire conversation on ReviewNB

Copy link
Collaborator

It doesn't appear to have made any difference to the times (or perhaps its slower?) so without .load is fine


View entire conversation on ReviewNB

Copy link

review-notebook-app bot commented Jul 10, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-10T00:28:48Z
----------------------------------------------------------------

Line #1.    %%time

There doesn't appear to be a need to time these three cells


navidcy commented on 2024-07-11T14:21:09Z
----------------------------------------------------------------

deal

Copy link

review-notebook-app bot commented Jul 10, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-10T00:32:24Z
----------------------------------------------------------------

Maybe change 'the same 1 degree longitude-latitude grid. '

to "a 1 degree longitude-latitude grid with regular spacing." ?

Maybe "In this example, " is better than "In particular, "


navidcy commented on 2024-07-11T14:19:58Z
----------------------------------------------------------------

Done

@anton-seaice
Copy link
Collaborator

Apologies for the double-review. This is very close :)

@navidcy
Copy link
Collaborator Author

navidcy commented Jul 10, 2024

@anton-seaice, review is a continuing process! we expect review comments until the community is happy -- there is no quota on reviews!

@navidcy
Copy link
Collaborator Author

navidcy commented Jul 11, 2024

@anton-seaice I'll skip black. I get this:

(base) nc3020@gadi-login-07:~/gdata/cosima-recipes$ black Examples/Horizontal_Regridding.ipynb
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install "black[jupyter]"``
No Python files are present to be formatted. Nothing to do 😴

and even after installing via pip install "black[jupyter]" --user I still get the same error...

@navidcy
Copy link
Collaborator Author

navidcy commented Jul 11, 2024

I did everything except the black formatting.

We could set up a GitHub action to apply black to all notebooks? I'm not sure if this is a good idea... Sometimes black makes things less pretty. It's also subjective...

Copy link
Collaborator Author

navidcy commented Jul 11, 2024

Done


View entire conversation on ReviewNB

Copy link
Collaborator Author

navidcy commented Jul 11, 2024

OK, done


View entire conversation on ReviewNB

Copy link
Collaborator Author

navidcy commented Jul 11, 2024

hm... I can't... can we leave this for some other time?


View entire conversation on ReviewNB

Copy link
Collaborator Author

navidcy commented Jul 11, 2024

deal


View entire conversation on ReviewNB

Copy link

review-notebook-app bot commented Jul 11, 2024

View / edit / reply to this conversation on ReviewNB

anton-seaice commented on 2024-07-11T23:12:12Z
----------------------------------------------------------------

We can get rid of the geolon/ geolat warnings by either not renaming 'geolon' to longitude and 'geolat' to latitde, or by updating the attributes when we do? Thoughts?

xesmf should understand cf attributes, so i think we can leave the names as geolon/geolat and it will know what to do


navidcy commented on 2024-07-19T03:08:36Z
----------------------------------------------------------------

done

@navidcy
Copy link
Collaborator Author

navidcy commented Jul 17, 2024

We can get rid of the geolon/ geolat warnings by either not renaming 'geolon' to longitude and 'geolat' to latitde, or by updating the attributes when we do? Thoughts?

xesmf should understand cf attributes, so i think we can leave the names as geolon/geolat and it will know what to do

Are you sure about this?

I thought these warnings are related to #150 and #212 because some coords are missing from the 1deg grid saved in ik11. But I may be wrong... I was merely guessing tbh.

@anton-seaice
Copy link
Collaborator

Im moderately confident its because of these lines:

ssh_1 = ssh_1.rename(
    {"xt_ocean": "x", "yt_ocean": "y", "geolon_t": "longitude", "geolat_t": "latitude"}
)

but the cf-attributes still refer to 'geolon_t' and 'geolat_t', which don't exist, so theres an error ?

Copy link
Collaborator Author

navidcy commented Jul 19, 2024

done


View entire conversation on ReviewNB

@navidcy
Copy link
Collaborator Author

navidcy commented Jul 19, 2024

@anton-seaice I believe this is ready

@navidcy navidcy changed the title Intake conversion Regridding; take #2 Intake conversion Regridding example; take #2 Jul 19, 2024
Copy link
Collaborator

@anton-seaice anton-seaice left a comment

Choose a reason for hiding this comment

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

Looks great! Good work :)

@navidcy navidcy merged commit 23c5b56 into main Jul 22, 2024
3 checks passed
@navidcy navidcy deleted the ncc/intake_regridding branch July 22, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛸 updating An existing notebook needs to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants