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 composite_analysis and xr_ttests #16

Merged
merged 14 commits into from May 29, 2019
Merged

add composite_analysis and xr_ttests #16

merged 14 commits into from May 29, 2019

Conversation

aaronspring
Copy link
Collaborator

create composite from timeseries and field.
check whether composite is different from mean with ttest

composite_analysis(
    pCO2, enso, plot=True, threshold=1, robust=True)

composite

@bradyrx
Copy link
Owner

bradyrx commented May 23, 2019

@aaronspring I need to actually review these two PRs because they look very useful. Been so focused on climpred I forgot about this stuff.

@aaronspring
Copy link
Collaborator Author

no worries. Would be nice to chat about those. It has been ages. Used it for some supplementary plot in my paper draft

@aaronspring
Copy link
Collaborator Author

Thanks for the revision and adapting to the updated code. Can this now go into master? LGTM

@bradyrx
Copy link
Owner

bradyrx commented May 28, 2019

Those were a few preliminary edits. Checking that this works with CESM output and might refactor slightly, then will merge.

@bradyrx
Copy link
Owner

bradyrx commented May 28, 2019

Will request a review once I'm done with this. Have a few more ideas.

@bradyrx
Copy link
Owner

bradyrx commented May 28, 2019

@aaronspring , give this a look and let me know what you think. This is ready to merge unless we want to clean up the warnings catch. Also, ttest is sort of slow, but there's nothing we can do about that I think.

nobs2,
input_core_dims=[[], [], [], [], [], []],
output_core_dims=[[], []],
vectorize=True, dask='parallelized')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

general comment: I am unsure whether parallelized is really a good idea.
Tip on http://xarray.pydata.org/en/stable/dask.html suggests:

For the majority of NumPy functions that are already wrapped by Dask, it’s usually a better idea to use the pre-existing dask.array function, by using either a pre-existing xarray methods or apply_ufunc() with dask='allowed'. Dask can often have a more efficient implementation that makes use of the specialized structure of a problem, unlike the generic speedups offered by dask='parallelized'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will have to investigate this for xskillscore in detail. I think it doesnt really change much, but I cannot prove it yet

Copy link
Owner

Choose a reason for hiding this comment

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

I can try running this on CESM data with parallelized and allowed and see timing differences. The issue is a dask array can't be put through the function because earlier lines of code (before the t-test) require it to be loaded into memory.

Copy link
Collaborator Author

@aaronspring aaronspring left a comment

Choose a reason for hiding this comment

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

the warnings catch is ok to me. I cannot review as the PR is mine.

@aaronspring aaronspring requested a review from bradyrx May 29, 2019 12:49
@bradyrx
Copy link
Owner

bradyrx commented May 29, 2019

@aaronspring, I'll merge this now then. You can check on the parallelized vs. allowed and let me know what you find. Thanks for this... I really like this function. Will make composites so much easier! Great work! Looking forward to developing this package a bit now with our experience from climpred.

@bradyrx bradyrx merged commit 3827288 into master May 29, 2019
@bradyrx bradyrx deleted the AS_composite_analysis branch May 29, 2019 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants