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

Add example of apply_ufunc #421

Merged
merged 7 commits into from
Aug 18, 2024
Merged

Add example of apply_ufunc #421

merged 7 commits into from
Aug 18, 2024

Conversation

jemmajeffree
Copy link
Collaborator

I've written up an example of how to calculate the trend and significance at each gridpoint using xr.apply_ufunc to pass things through to scipy.stats.linregress, so it's vectorised and much faster than using a for loop

Is this something that's of use to the COSIMA community?

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@navidcy navidcy changed the title add apply_ufunc example Add example of apply_ufunc Jul 10, 2024
@ashjbarnes
Copy link
Collaborator

This looks great! I've added a comment to suggest a bit more motivation at the top of the notebook :)

@anton-seaice
Copy link
Collaborator

anton-seaice commented Jul 10, 2024

Thankyou for doing this Jemma!

I think this could go in Tutorials rather than Examples ?

(I will also point out xarray has a polyfit which does the same as the notebook :) )

If we move to the intake catalog, then the chunking warnings will go away :)

And it would be good to fix up the figure to be tripole aware ... although I guess its not actually essential.

Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

@jemmajeffree thanks!

I pushed few edits... space after commas etc

the PR looks good to me! I run the notebook and it's all good!

I just don't understand what np.arange(sst.time.shape[0])/12 does...
Seems something hardcoded to me... something related with months (or the conspicuous coincidental 12)... but I have no idea.

If we can reduce the hardcoding it'd be great. Otherwise a comment next to it explaining what's happening will be useful for somebody reading this example down the road. But I am happy to merge this even as is rather than keep ruminating over details!

@navidcy
Copy link
Collaborator

navidcy commented Jul 19, 2024

Also @ashjbarnes suggestion is nice. If you can incorporate this before merging it'd be nice!

@navidcy
Copy link
Collaborator

navidcy commented Aug 16, 2024

@jemmajeffree I think I'll include a sentence along what @ashjbarnes suggested and merge. Unless you plan to edit/work on this?

@navidcy
Copy link
Collaborator

navidcy commented Aug 16, 2024

(We can always open a new PR in the future and revisit-enhance the recipe!)

@jemmajeffree
Copy link
Collaborator Author

I've updated the example here /g/data/x77/jj8842/cosima-recipes/Examples/Apply_scipy_function_to_every_gridpoint.ipynb, but I can't work out how to push it back to the PR and merge with existing changes here. I can try follow instructions, or I'm very happy to have someone else work out how to merge them again.

I've:
- added some text as suggested by @ashjbarnes
- tried to clarify what is meant by the np.arange(sst.time.shape[0])/12), which is specific to the linear regression, so I guess hardcoded to this example but I can't see how you'd generalise when you don't know what other functions might be used with this code and what variables they would take

Also noting the comments by @anton-seaice, I know you could do this specific task faster with xr.polyfit, but some things you can't and linregress is an approachable example

Feel free to rewrite any of my explanations, or add anything you think they're lacking

@navidcy
Copy link
Collaborator

navidcy commented Aug 18, 2024

We can zoom and I show you?

@navidcy
Copy link
Collaborator

navidcy commented Aug 18, 2024

oh I see... probably you can't push because I pushed few edits and you need to merge them! damn... I made it hard....

@navidcy
Copy link
Collaborator

navidcy commented Aug 18, 2024

I'll sort it out!

@navidcy navidcy merged commit 7d4f139 into COSIMA:main Aug 18, 2024
2 checks passed
@navidcy
Copy link
Collaborator

navidcy commented Aug 18, 2024

@jemmajeffree
Copy link
Collaborator Author

Thanks @navidcy for sorting that out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants