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 more fill variants #1310

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Add more fill variants #1310

merged 1 commit into from
Nov 15, 2023

Conversation

dbalagansky
Copy link
Contributor

I'm not an expert in numpy, so this was borrowed from SO: https://stackoverflow.com/a/54508024.

I probably should rename this from "previous" to "similar" (or maybe some other better word?), as this thing covers the case when first point in resulting series could be NaN with backwards fill in addition to the standard forward fill.

Fixes #1266

Copy link
Contributor

@tobias-urdin tobias-urdin left a comment

Choose a reason for hiding this comment

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

Please add testing to show functional behaviour, a release note would be good to, see [1] for using reno to add a release note.

[1] https://docs.openstack.org/reno/latest/user/usage.html#creating-new-release-notes

@tobias-urdin
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2023

rebase

❌ Unable to rebase: user tobias-urdin is unknown.

Please make sure tobias-urdin has logged in Mergify dashboard.

@tobias-urdin
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 1, 2023

rebase

✅ Branch has been successfully rebased

@dbalagansky dbalagansky changed the title Add "previous" fill variant Add "similar" fill variant Aug 1, 2023
@tobias-urdin tobias-urdin requested a review from chungg August 1, 2023 12:13
Copy link
Member

@chungg chungg left a comment

Choose a reason for hiding this comment

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

sort of surprised we didn't have this but you should update doc/source/rest.j2 as well.

not blocker but:

  • similar is a odd keyword. not sure if there's existing keyword we can copy from other libs? bidirectional seems more accurate but maybe too long?
  • would be good to have forward and backward as well. i suspect forward is the most common option.

@dbalagansky
Copy link
Contributor Author

dbalagansky commented Sep 29, 2023

  • would be good to have forward and backward as well. i suspect forward is the most common option.

I've been thinking about these for some time and the problem with plain backward and forward fill variants is that, given this series: [NaN, NaN, 1, 2, NaN, 4, 5, NaN, NaN]:

  • doing just forward fill would result in: [NaN, NaN, 1, 2, 2, 4, 5, 5, 5]
  • doing just backward fill would result in: [1, 1, 1, 2, 4, 4, 5, NaN, NaN]

So, as you can see, in some cases it wouldn't be enough to do forward or backward fill to produce fully populated series (without NaN's) and, maybe, it would be a good idea to:

  • add something like full_ffill for doing forward than backward (for overwriting first two NaN's in the example above after normal forward fill) and same thing with full_bfill: backward than forward fill. This way there would be no need for similar or bidirectional fill variants as the one that is implemented in this PR is just full_ffill one
  • add ffill and bfill, which would leave NaN's in the resulting series alone

So maybe implementing one of the above (or both?) would be a better option in the end?

@chungg
Copy link
Member

chungg commented Oct 23, 2023

* add something like `full_ffill` for doing `forward` than `backward` (for overwriting first two `NaN`'s in the example above after normal `forward` fill) and same thing with `full_bfill`: `backward` than `forward` fill. This way there would be no need for `similar` or `bidirectional` fill variants as the one that is implemented in this PR is just `full_ffill` one

* add `ffill` and `bfill`, which would leave `NaN`'s in the resulting series alone

yeah, this makes sense to me. i didn't even consider the differences in order of your full fill solution. i'm ok with the naming. don't have anything better to suggest

@dbalagansky dbalagansky changed the title Add "similar" fill variant Add more fill variants Nov 3, 2023
@dbalagansky
Copy link
Contributor Author

dbalagansky commented Nov 3, 2023

This should be ready for review, hope I didn't miss any points, but feel free to point them out :)

One more thing that came to my mind during the final tests was that this probably should interact with resampling down (e.g., doing aggregates resample mean 5 on a metric with measurements every minute) by creating additional measurements and filling them, but that sound like a separate PR.

Copy link
Member

@chungg chungg left a comment

Choose a reason for hiding this comment

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

lgtm. minor wording update

doc/source/rest.j2 Outdated Show resolved Hide resolved
doc/source/rest.j2 Outdated Show resolved Hide resolved
@tobias-urdin tobias-urdin merged commit aa2594c into gnocchixyz:master Nov 15, 2023
5 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Ability to set non-static fill value, based on existing data in series for aggregates
3 participants