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

Use min views filter = 1 #1754

Merged
merged 5 commits into from
Apr 12, 2024
Merged

Use min views filter = 1 #1754

merged 5 commits into from
Apr 12, 2024

Conversation

pierotofy
Copy link
Member

Opening draft for testing and possible merging.

So far I have not seen significant increases in noise, all while coverage for lower overlap areas is significantly improved.

@pierotofy
Copy link
Member Author

pierotofy commented Apr 10, 2024

I also propose to change the default values for pc-filter from 2.5 to 20 5 and to use dspsift by default as the feature-type; 2.5 is too aggressive and tends to remove points near facades (when shot nadir), so buildings don't get reconstructed properly. A value of 5 removes far outliers, but preserves more facade points. dspsift does not take advantage of the GPU, but performs better than sift, so it should be the default.

@pierotofy pierotofy marked this pull request as ready for review April 10, 2024 16:54
@smathermather
Copy link
Contributor

dsp-sift is overdue. It's a harmless to useful change. And I'll remember to use it if its default, so bonus.

Haven't tested the changes in filtering yet, but min-consistent-views change looks fine, even in datasets with excessive existing overlap. Seems to reduce noise marginally in very high-overlap datasets because it fills gaps:

image

@smathermather
Copy link
Contributor

I'll post these here too from https://community.opendronemap.org/t/processing-historical-aerial-photos-revisited/19273/44?u=smathermather
image
image
image

@pierotofy
Copy link
Member Author

anim

@smathermather
Copy link
Contributor

20 was not aggressive enough, I assume?

Definitely nicer completeness on that ballfield.

@pierotofy
Copy link
Member Author

20 was not aggressive enough, I assume?

Yes, as usual more testing yielded some counterexamples of where being too soft on filtering left some undesirable results.

@smathermather
Copy link
Contributor

smathermather commented Apr 10, 2024

Leaf off forest example (just min-num change, not filtering change yet -- too many test datasets now running and a few hours in...):

ezgif-2-3cb2d97242
ezgif-2-3296c48dd7

@smathermather
Copy link
Contributor

dspsift does not take advantage of the GPU, but performs better than sift, so it should be the default.

Should we continue to default to sift when GPU is available, or not?

@Saijin-Naib
Copy link
Contributor

Mmm... I think GPU SIFT should be opt-in until it matches CPU SIFT/DSPSIFT reconstruction.

I think getting better results slower is a kinder default than less complete results faster.

Easier to explain and adjust folks to, as well, I would think.

@smathermather
Copy link
Contributor

smathermather commented Apr 10, 2024

Hmm, good point. This will improve outcomes for GPU users as is. 🙂

@smathermather
Copy link
Contributor

Nice to see vegetation much better handled (not yet using the new filter value, but re-running with that...:
image

@smathermather
Copy link
Contributor

Confirmed that filter value does help with this relatively low overlap dataset on building edge:
image

(here in combination with the min views filter = 1)

@pierotofy
Copy link
Member Author

I've run some tests on a few datasets and I'm good to merge this, unless there are objections or further testing presents some issues.

@smathermather
Copy link
Contributor

I've got one more dataset to test and then I'm happy to bless it too.

@smathermather
Copy link
Contributor

Hitting an OOM on a 200+ image dataset at DEM stage on a 768GB RAM machine. Probably something degenerate with the point cloud (will look tomorrow / later today).

@pierotofy -- have you done any memory profiling in your tests? I didn't think till now to try some docker instances with capped RAM for verifying.

@pierotofy
Copy link
Member Author

Mm, good point. I did find an out of memory issue (during the depthmap fusion step). Will need to investigate a bit.

@Saijin-Naib
Copy link
Contributor

The difference is subtle with my incredibly poorly collected mf_old-orchard, but it is better:
image

vs

image


image

vs

image

So, vegetation and built structures seem to benefit most, which is great, especially at the borders of the survey (I had no overfly in my survey designs back then).

There is an incredibly slight increase in total inaccuracy, but estimated GSD also very slightly improves.

I think it is overall a huge net-positive.

@pierotofy
Copy link
Member Author

pierotofy commented Apr 12, 2024

I've processed the dataset that was giving me memory troubles (I think I was running out of RAM because I had QGIS open, not necessarily because of the large increase in memory). Just to be safe, I've improved the mechanism for kicking in the clustering fusion logic.

This PR will increase the number of points for every dataset and thus the memory requirements a bit, but not severely. I processed all the large datasets that push the limits of my dated development machine without issues. So I'm OK to merge this.

@smathermather
Copy link
Contributor

Cool. In the absence of the forthcoming test suite that I hope will A/B test changes in memory (time tbd, but after May), I have no objection to a merge.

It makes a big difference in a lot of datasets.

@pierotofy pierotofy merged commit 6084d1d into OpenDroneMap:master Apr 12, 2024
2 checks passed
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.

None yet

3 participants