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

Remove a duplicate histogram calculation #905

Merged
merged 3 commits into from
Sep 1, 2021

Conversation

markccchiang
Copy link
Contributor

It is my fault that introduced this bug in PR 790. The image histogram configs cache should be cleared when resetting the histogram requirements for a Frame.

@confluence
Copy link
Collaborator

What is the impact of this -- do we just send an extra histogram? Does this need to be fixed in the current beta release, or can it wait until the next one?

Please also add this to the changelog.

@markccchiang
Copy link
Contributor Author

What is the impact of this -- do we just send an extra histogram? Does this need to be fixed in the current beta release, or can it wait until the next one?

Please also add this to the changelog.

It just sends an extra histogram. Since this extra histogram calculation has been cached, it spends at most a few milli-secondes to do so. I think it can wait until the next one.

@confluence
Copy link
Collaborator

Long-term suggestion: we wouldn't have this problem if these containers were sets rather than vectors -- but then we'd have to make sure the objects we put in them are hashable (possibly by specifying an appropriate hash function).

Copy link
Collaborator

@confluence confluence left a comment

Choose a reason for hiding this comment

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

This new changelog entry needs to go in a new [Unreleased] section -- we have already tagged the last release.

@veggiesaurus veggiesaurus merged commit 25137b5 into dev Sep 1, 2021
@veggiesaurus veggiesaurus deleted the mark/remove_duplicate_histogram_calculations branch September 1, 2021 10:36
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.

3 participants