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

Adds new absmax aggregation method #76

Merged
merged 8 commits into from Jun 27, 2017
Merged

Conversation

yadsirhc
Copy link
Contributor

@yadsirhc yadsirhc commented May 9, 2014

absmax is a new aggregation method which returns the largest
absolute value when aggregating to a lower precision archive.

Useful for data such as time offsets, where you care about retaining
the value furthest from zero when aggregating but would prefer to
preserve whether the offset was positive or negative.

@steve-dave
Copy link
Member

@yadsirhc Does having a absmin function make any sense at all? If so, perhaps package it in with this PR?

@yadsirhc
Copy link
Contributor Author

I have yet to encounter a use case firsthand where an absmin function would make sense. I did consider adding one for the sake of symmetry, but soon realized that it would not be able to leverage built-in Python aggregation functions, so it felt like overkill.

@steve-dave
Copy link
Member

@yadsirhc Python is not my expertise, but would this be too inefficient:

return min(knownValues, key=abs)

If Python can do that efficiently then it might also be possible to simplify your implementation of absmax with:

return max(knownValues, key=abs)

It all depends on how efficiently Python implements abs.

@yadsirhc
Copy link
Contributor Author

Thanks for the suggestion. For small arrays it appears to be a negligible performance hit (~50 ns) for absmax versus the prior commit, but the code is much cleaner. In the case of absmin, I doubt another reimplementation could beat this one.

@steve-dave
Copy link
Member

@esc does this look mergeable to you?

@SEJeff
Copy link
Member

SEJeff commented Jun 17, 2015

@yadsirhc can you please rebase this to the latest changes in whisper.git master?

absmax is a new aggregation method which returns the largest
absolute value when aggregating to a lower precision archive.

Useful for data such as time offsets, where you care about retaining
the value furthest from zero when aggregating but would prefer to
preserve whether the offset was positive or negative.
Chris Day added 2 commits June 24, 2015 14:13
Also refactors absmax to use a lambda function with max.
@yadsirhc
Copy link
Contributor Author

@SEJeff Done. Also updated the README to reflect avg_zero since it was the reason for the conflicts but hadn't itself been documented.

@deniszh
Copy link
Member

deniszh commented Aug 2, 2016

@yadsirhc - could you please rebase your change if still needed?

Chris Day added 4 commits August 2, 2016 17:52
absmax is a new aggregation method which returns the largest
absolute value when aggregating to a lower precision archive.

Useful for data such as time offsets, where you care about retaining
the value furthest from zero when aggregating but would prefer to
preserve whether the offset was positive or negative.
Also refactors absmax to use a lambda function with max.
@yadsirhc
Copy link
Contributor Author

yadsirhc commented Aug 2, 2016

Rebased, as requested.

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage decreased (-0.2%) to 88.83% when pulling 1fac0ff on yadsirhc:master into 1e96c0c on graphite-project:master.

@deniszh deniszh added the rebase label Mar 19, 2017
@yadsirhc
Copy link
Contributor Author

No conflicts remain with the base branch. Is the rebaseneeded label still appropriate? What is preventing merge now (or back in Aug 2016, for that matter)?

@deniszh
Copy link
Member

deniszh commented May 31, 2017

Hello @yadsirhc,
Sorry for very long merging time, we're trying to maintain Graphite properly now.
Will merge this today or tomorrow, no worries.
Does anyone want to review too? /cc @graphite-project/committers

@DanCech DanCech assigned DanCech and unassigned DanCech May 31, 2017
@DanCech DanCech self-requested a review May 31, 2017 14:49
@DanCech DanCech merged commit 0360c0d into graphite-project:master Jun 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants