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

[WIP] Make sure xarray.polyfit does not rechunk datarray #117

Closed
wants to merge 1 commit into from
Closed

[WIP] Make sure xarray.polyfit does not rechunk datarray #117

wants to merge 1 commit into from

Conversation

navidcy
Copy link
Contributor

@navidcy navidcy commented Oct 30, 2020

Out of principle, xrft should not be rechunking the users input on the background. This PR is a "hack" for what supposed to be an issue with xarray.polyfit() (see #116 and pydata/xarray#4554). xarray.polyfit() seems to be rechunking the data and xrft calls xarray.polyfit() when the user asks for a linear detrend of the input of xrft.dft(). The proposed change simply makes sure that the detrended data array has the same chunking as the original one.

Closes #116

@navidcy navidcy changed the title Make sure xarray.polyfit does not rechunk datarray [WIP] Make sure xarray.polyfit does not rechunk datarray Oct 30, 2020
@codecov
Copy link

codecov bot commented Oct 30, 2020

Codecov Report

Merging #117 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #117   +/-   ##
=======================================
  Coverage   98.31%   98.31%           
=======================================
  Files           2        2           
  Lines         830      831    +1     
  Branches      118      118           
=======================================
+ Hits          816      817    +1     
  Misses          7        7           
  Partials        7        7           
Impacted Files Coverage Δ
xrft/xrft.py 96.82% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3662a85...5ae0fad. Read the comment docs.

Copy link
Collaborator

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Thanks for trying this @navidcy. However, I don't think the fix can possibly be so easy.

@@ -141,7 +141,8 @@ def _apply_detrend(da, dim, axis_num, detrend_type):
if len(dim) == 1:
p = da.polyfit(dim=dim[0], deg=1)
linear_fit = xr.polyval(da[dim[0]], p.polyfit_coefficients)
return da - linear_fit
da_detrended = (da - linear_fit).chunk(da.chunks) # see discussion in https://github.com/xgcm/xrft/issues/116
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not fix the problem. Instead this just creates a new layer of chunks in the task graph. The "upstream" tasks, which have the problems described in #116, still exist and have to be computed before we get to the new chunks. So actually this makes things worse by increasing the total number of tasks.

@navidcy navidcy closed this Oct 30, 2020
@rabernat
Copy link
Collaborator

FYI, I am working on this a bit.

@navidcy navidcy deleted the polyfit-chunking branch November 16, 2020 20:11
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.

Undesired chunking of 3d input when using linear detrend with poyfit()
2 participants