Add support for coordinate inputs in polyfit.#9369
Conversation
|
Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient. |
|
@aulemahal are you able to take a look here |
aulemahal
left a comment
There was a problem hiding this comment.
This looks good! Not sure if I'm entitled to approve haha.
I put a comment that does not really concern this PR. Simply that it seems we could take this opportunity to clean up some older stuff.
xarray/core/dataset.py
Outdated
| skipna_da = skipna | ||
|
|
||
| x = get_clean_interp_index(self, dim, strict=False) | ||
| x = get_clean_interp_index(self, dim, use_coordinate=dim, strict=False) |
There was a problem hiding this comment.
This code is a bit strange. I think the get_clean_interp_index evolved in a way that makes it not match its signature and doc.
Argument "dim" of get_clean_interp_index is documented as "Name of the dimension", yet here the name of the coordinate is passed (which might not be a dimension, that's the point). However, the dim argument has no impact whatsoever on the the result of the function if use_coordinate is a string.
Also, the docstring of use_coordinate does not mention what happens when a string is passed.
There was a problem hiding this comment.
Agreed that it is strange, in an ideal world I think the dim argument could be either a coordinate or a dimension, and use_coordinate just wouldn't be necessary. But I didn't want to change the get_clean_interp_index signature too much since it seemed to match the public interpolate_na API quite closely, which I'm not familiar with.
I also didn't set the dim argument to None because there seemed to be some downstream use of that variable in the case where the resulting index is a DatetimeIndex.
Maybe as a half measure I could update the documentation of get_clean_interp_index in this PR to match more closely what the code is actually doing?
Alternatively, I could just inline the behaviour of get_clean_interp_index that we're actually using since that function seemed to be designed for a different purpose anyway.
There was a problem hiding this comment.
I could just inline the behaviour of get_clean_interp_index that we're actually using since that function seemed to be designed for a different purpose anyway.
This would be great if it's a simple add
There was a problem hiding this comment.
Done :) although mypy complained at me initially for reassigning x to different types. I'm not sure how precise this project wants all its types to be so for now I've just declared x as Any for the sake of brevity.
There was a problem hiding this comment.
FYI: I am currently working on improvements of interpolate_na and discovered similar issues with get_clean_interp_index. I tried to come up with an improved (and type safe) version of it (see here). However, good to know that polyfit is now independent, in this case get_clean_interp_index is only used within missing.py. I will try to write a better docstring for the function before merging.
for more information, see https://pre-commit.ci
|
Just bumping this PR, let me know if I need to make any additional changes to get this merged. :) |
|
Any final comments before we merge? (thank you @Karl-Krauth !) |
|
We never merged, sorry @Karl-Krauth . Could we fix the pre-commit error? Then ping us if no one hits the button... |
Use "raise from" when dimensions aren't castable to float in polyfit.
|
No worries! Yeah, looks like this was a failure from ruff that wasn't triggered when I first opened the PR. Should be fixed now :) |
|
Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! |
whats-new.rst