8000 Avoid unnecessary copy of queue in IncrementalTriangulator::Complete() by ahojnnes · Pull Request #2764 · colmap/colmap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Avoid unnecessary copy of queue in IncrementalTriangulator::Complete() #2764

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 9, 2024
Merged

Avoid unnecessary copy of queue in IncrementalTriangulator::Complete() #2764

merged 1 commit into from
Sep 9, 2024

Conversation

ahojnnes
Copy link
Contributor
@ahojnnes ahojnnes commented Sep 9, 2024

The order in which the points are completed does not matter, so the change of FIFO to LIFO queue produces the same results.

@ahojnnes ahojnnes merged commit 18dc756 into main Sep 9, 2024
16 checks passed
@ahojnnes ahojnnes deleted the user/jsch/triangulation-speedups branch September 9, 2024 12:38
@BrewTestBot BrewTestBot mentioned this pull request Nov 28, 2024
Merged
@B1ueber2y
Copy link
Contributor

Not sure how we overlooked this. But this update does not seem correct? The second level of transitivitiy will never get executed since the queue is already empty.

@ahojnnes
Copy link
Contributor Author
ahojnnes commented Jan 3, 2025

I'll have a look tonight. How did you notice it? Different behavior observed?

@ahojnnes
Copy link
Contributor Author
ahojnnes commented Jan 3, 2025

Please double check: #3094

I just evaluated this fix against the main branch with the new benchmark suite and the A/B comparison looks like this. It's still a bug that should be fixed.

I20250103 17:19:16.339394 3018444 compare.py:main:58] Results A - B:
=====scenes===== ======AUC @ X deg (%)====== ===images=== =components=
                  0.5    1.0    5.0    10.0     reg   all  num largest

==============================eth3d=dslr==============================
botanical_garden   0.00   0.00   0.00   0.00      0     0    0       0
boulders           0.00   0.00   0.00   0.00      0     0    0       0
bridge            -0.00  -0.00  -0.00  -0.00      0     0    0       0
courtyard          0.00   0.00   0.00   0.00      0     0    0       0
delivery_area      0.00   0.00   0.00   0.00      0     0    0       0
door               0.00   0.00   0.00   0.00      0     0    0       0
electro           -0.00  -0.00  -0.00  -0.00      0     0    0       0
exhibition_hall    0.00   0.00   0.00   0.00      0     0    0       0
facade            -0.01  -0.01  -0.00  -0.00      0     0    0       0
kicker             0.00   0.00   0.00   0.00      0     0    0       0
lecture_room      -0.00  -0.00  -0.00  -0.00      0     0    0       0
living_room        0.00   0.00   0.00   0.00      0     0    0       0
lounge             0.00   0.00   0.00   0.00      0     0    0       0
meadow             0.00   0.00   0.00   0.00      0     0    0       0
observatory       -0.00  -0.00  -0.00  -0.00      0     0    0       0
office             0.00   0.00   0.00   0.00      0     0    0       0
old_computer       0.00   0.00   0.00   0.00      0     0    0       0
pipes              0.00   0.00   0.00   0.00      0     0    0       0
playground        -0.00   0.00   0.00   0.00      0     0    0       0
relief            -0.00  -0.00  -0.00  -0.00      0     0    0       0
relief_2          -0.00  -0.00  -0.00  -0.00      0     0    0       0
statue             0.00   0.00   0.00   0.00      0     0    0       0
terrace           -0.00  -0.00  -0.00  -0.00      0     0    0       0
terrace_2         -0.00  -0.00  -0.00  -0.00      0     0    0       0
terrains          -0.00  -0.00  -0.00  -0.00      0     0    0       0
----------------------------------------------------------------------
overall           -0.00  -0.00  -0.00  -0.00      0     0    0       0
----------------------------------------------------------------------
average           -0.00  -0.00  -0.00  -0.00      0     0    0       0

@B1ueber2y
Copy link
Contributor

I'll have a look tonight. How did you notice it? Different behavior observed?

No different behavior observed. Noticed since another project was looking into the Complete operation.

I just evaluated this fix against the main branch with the new benchmark suite and the A/B comparison looks like this.

I think the previous logic was basically infinite depth transitivity since the transitivity was not updated. So even if there is different in A/B test, fixing this bug can only make the results worse but can potentially lead to faster results (which is not reflected in the test). The bug should be fixed in the sense that the complete_max_transitivity was not useful before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@B1ueber2y B1ueber2y B1ueber2y approved these changes

@sarlinpe sarlinpe Awaiting requested review from sarlinpe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ahojnnes @B1ueber2y
0