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

Apparent bug in blob pruning code #2022

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Apparent bug in blob pruning code #2022

wants to merge 2 commits into from

Conversation

evariste
Copy link

When pruning blobs that have overlap, some blobs can be removed when comparing with a blob that has previously been removed. Also, blobs with a negative radius, set in an earlier iteration are not passed to the overlap calculation function.

@@ -92,6 +92,13 @@ def _prune_blobs(blobs_array, overlap):
# iterating again might eliminate more blobs, but one iteration suffices
# for most cases
for blob1, blob2 in itt.combinations(blobs_array, 2):

# Ignore if we have removed either blob during an earlier iteration.
if blob1[3] == -1:
Copy link
Member

Choose a reason for hiding this comment

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

if (blob1[3] == -1) or (blob2[3] == -1)):

Copy link
Author

Choose a reason for hiding this comment

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

Yes, of course.

Copy link
Author

Choose a reason for hiding this comment

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

But I've just realised that it should actually be

if (blob1[2] == -1) or (blob2[2] == -1)):

Because the 'radius' is at index 2. Apologies.

@stefanv
Copy link
Member

stefanv commented Mar 23, 2016

The code seems to do the radius detection, but I don't see the first step that you mention in the PR description? Also, I want to double check: this is an optimization, not a bug, right?

@stefanv stefanv added the ⏩ type: Enhancement Improve existing features label Mar 23, 2016
@evariste
Copy link
Author

Hi Stefan. The situation I had can be described in this example. In the original code, say two blobs, A and B are compared for overlap and let's assume B is correctly marked for removal. This is done by setting its 'radius' to -1. Later B is compared with C but their overlap is now measured assuming that B has a radius of -1. This can give an incorrect result and lead to C being incorrectly marked for removal.

@soupault
Copy link
Member

soupault commented Apr 9, 2016

@evariste the change looks good. Would you mind creating a test to cover this case?
Am I right that it has to be presented for the area in #1831 (comment) ?

Base automatically changed from master to main February 18, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants