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 |
xarray/core/dataset.py
Outdated
| @@ -9023,7 +9023,10 @@ def polyfit( | |||
| variables = {} | |||
| 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 !) |
whats-new.rst