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

Rolling min max #22

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

qchateau
Copy link

Rolling min and max with insertion complexity O(logN), where N is the window size, and extraction complexity O(1).

Is this fit for a merge into the main library ?

To be fair, I don't even know if this is the right place to submit a patch, but I could not find any information on how to contribute to accumulators. I hope someone can help.

doc/accumulators.qbk Outdated Show resolved Hide resolved
doc/accumulators.qbk Outdated Show resolved Hide resolved
@yuvalif
Copy link
Contributor

yuvalif commented Dec 24, 2018

@tytan what if someone just wants min/max but without sorted rolling window? in this case insertion complexity should be O(1). only dependency should be of rolling window, not the sorted one.

@qchateau
Copy link
Author

I will fix the documentation. What would you say is the quickest way of generating a human readable version of the doc ?

I sure can implement a lazy and immediate version of the rolling min and max if you deem the complexity acceptable. Anyway, you are right, people should be allowed to have the choice.

In this case what would you say should be the default implementation ? The lazy one would not mess with insertion cost, but I think people expect extraction to be fast.

@yuvalif
Copy link
Contributor

yuvalif commented Dec 24, 2018

I will fix the documentation. What would you say is the quickest way of generating a human readable version of the doc ?

I have no idea... maybe ask on the mailing list?

I sure can implement a lazy and immediate version of the rolling min and max if you deem the complexity acceptable. Anyway, you are right, people should be allowed to have the choice.
In this case what would you say should be the default implementation ? The lazy one would not mess with insertion cost, but I think people expect extraction to be fast.

In general insertion cost is more important than extraction (assuming there are more insertions than extractions), but probably best to look at other accumulators where both options exist.
Would be even nicer (though I'm not sure how to do that...) if you can collapse the lazy implementation to the regular one if you need the ordered window anyway (e.g. combining lazy min with regular max)

@qchateau
Copy link
Author

I had to split the declaration of rolling_window and rolling_window_plus1 in rolling_window.hpp because GCC was complaining that the extractor of rolling_window used the extractor of rolling_window_plus1 before it was declared.

I added a lazy/immediate implementation for rolling min and max. The default being the lazy one.

I think addition of a mecanism to chose the immediate implementation instead of the lazy one if the sorted_rolling_window is used as a dependency by another feature in the set can be the subject of a further PR. I may work on this later on.

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.

2 participants