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 MEDIAN group function #4344

Merged
merged 3 commits into from
Aug 24, 2024
Merged

Add MEDIAN group function #4344

merged 3 commits into from
Aug 24, 2024

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Aug 11, 2024

Resolve #4341

@jimtng jimtng requested a review from a team as a code owner August 11, 2024 14:46
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/feature-request-median/157728/12

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM. Thanks!

@mherwege
Copy link
Contributor

mherwege commented Aug 11, 2024

@jimtng Great you took this one. I am wondering if it makes sense to use a more performant algorithm using heaps? See https://www.baeldung.com/java-stream-integers-median-using-heap. I expect it will for the other case with persistence when looking at a longer period with many data points. Here the impact could be important because this recalculation (and sorting) will be done for every state change of an item in the group.

@mherwege
Copy link
Contributor

And here is a text on another, at first side even better, algorithm: https://rcoh.me/posts/linear-time-median-finding/

@jimtng
Copy link
Contributor Author

jimtng commented Aug 12, 2024

@mherwege great idea! I'm going to do some tests comparing the two/three implementations.

I'm also wondering about keeping an in-memory sorted list and update it whenever the members changed. That would involve a sorting operation at the beginning, and only deletion + insertion of (changed|added|removed) item(s), which should probably be even faster than any quickselect / heap median algorithm. However, the way it's currently done, we got given a list of items with no knowledge of the groupitem (only the baseitem). So to do that, a lot of things would need to be changed.

@mherwege
Copy link
Contributor

@jimtng I am not sure the extra complexity of handling item state changes + item addition + item removal in a group are worth it for this use case. Using an algorithm that scales linearly (or at least most of the time) with number of items I suspect does.
Looking at the algorithms, the advantage of the heap methods is when you want to have a stream of medians with every item you add. It avoids sorting (or quickselect) every time. That is not the case here. You only need to calculate once for a change every time, so quickselect looks like the most promising. I of course applaud you intending to test it.

@mherwege
Copy link
Contributor

deletion + insertion of (changed|added|removed) item(s)

That’s what the heap approach could optimise.

@jimtng
Copy link
Contributor Author

jimtng commented Aug 14, 2024

Note: waiting for #4345 to be merged, and refactor to use the Statistics.median method.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@mherwege
Copy link
Contributor

@jimtng #4345 has been merged.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng
Copy link
Contributor Author

jimtng commented Aug 24, 2024

Thanks, updated!

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Aug 24, 2024
@J-N-K J-N-K added this to the 4.3 milestone Aug 24, 2024
@J-N-K J-N-K merged commit 8d54cce into openhab:main Aug 24, 2024
5 checks passed
@jimtng jimtng deleted the median-groupfunction branch August 24, 2024 12:41
@geert-claes
Copy link

Thanks y'all guys for implementing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Median as group function.
5 participants