Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fill gaps limited 7665 #9402
base: main
Are you sure you want to change the base?
Fill gaps limited 7665 #9402
Changes from all commits
bbfc476
16cdf30
43b7165
d46baa4
1fb7795
878e6bb
1ac5e9c
97b00a4
1626489
b3d70d6
6b4c0f7
84fe728
3bbd6da
3ec34bf
a64809a
92c6b2a
5452df1
2f53449
3696d63
d5d56ae
7570b62
f19f626
b5025ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts from others on the naming? Would
.fill
be insufficiently specific that it's fillingna
? Wouldfill_missing
be clearer thanfill_gaps
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to any of those.
.fill
sounds very concise, but maybe this is easily confused with.ffill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
.fill
could be quite nice, do others have a view?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
gap_filler
instead, since this method does not actually fill the gaps.I'm also wondering if its better to have a method that constructs the appropriate mask that can be used later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points! Just to explain:
gap_filler
emphasizes the returned object type nicely! However, I choosefill_gaps
because it fits the naming scheme of other object-returning functions better (e.g.rolling
andcoarsen
are not calledroller
andcoarser
in xarray, even though the operation is not perfomed immediately and an object is returned).Ultimately (I am a non-native english speaker) I am happy for any recommendations regarding nomenclature.
If you prefer
gap_filler
, I will change accordingly.The function API is also presented as an alternative in the initial proposal. I decided to go for the object way because it is shorter (one line) and less error prone (you might easily forget the
~
). If the mask is required, you can easily get it from the object: