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

Add bal dataloader to test SBT #251

Open
wants to merge 41 commits into
base: sparse/pr
Choose a base branch
from
Open

Conversation

hxu296
Copy link
Member

@hxu296 hxu296 commented Jun 15, 2023

This PR adds a BAL dataset loader at pypose/examples/sparse/bal_loader.py. fix: #235

This change includes 4 components:

  1. BAL data loader written in TorchData’s functional style: bal_loader.py
  2. BAL specific util functions for calculating reprojerr and visualizing loss: bal_utils.py
  3. Dense bundle adjustment solver using Adam optimizer: ba_dense.py
  4. Google Colab example, so anyone can try it painlessly: ba_example.ipynb (Demo)

@hxu296 hxu296 requested a review from zitongzhan June 15, 2023 04:36
@hxu296
Copy link
Member Author

hxu296 commented Jun 16, 2023

As discussed with Zitong, I will add a PyTorch SGD based BA algorithm to test the dataloader and also serve as a simple baseline for sparse LM based BA.

Also thank Zitong for going through the SBT implementations and sparse Jacobian plans with me, which helps me to catch up to speed :)

Copy link
Contributor

@huyaoyu huyaoyu left a comment

Choose a reason for hiding this comment

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

Only reviewed the bal_loader.py. Need some more time on ba_dense.py.

examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Show resolved Hide resolved
examples/sparse/bal_loader.py Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
@hxu296
Copy link
Member Author

hxu296 commented Jun 26, 2023

Thanks for your review @huyaoyu
ba_dense.py is still WIP as for now. I will ping you when it's ready for review, thanks!

@hxu296
Copy link
Member Author

hxu296 commented Jun 28, 2023

The CI failure looks weird, as commit 535e907 is scoped within examples/sparse...

@hxu296
Copy link
Member Author

hxu296 commented Jul 7, 2023

Addressed all comments regarding bal_loader.py. Added a new file bal_utils.py for a custom reprojerr function that considers radial distortions k1 and k2.

These two files are ready for review:

  1. bal_loader.py
  2. bal_utils.py

Meanwhile, I will keep working towards a bal_dense.py and a jupyter notebook example that is colab ready. Cheers.

@hxu296 hxu296 requested a review from huyaoyu July 7, 2023 07:08
@hxu296
Copy link
Member Author

hxu296 commented Jul 8, 2023

Hi, the following two files are ready for review, and nothing else is on the workbench.

  1. ba_dense.py
  2. ba_example.ipynb

You can try the jupyter notebook example through google colab by clicking on the "Open in Colab" tab in the README.

Here is a visualization of loss before and after optimization:

image

Here is a visualization of the loss history:

image

Cheers.

Copy link
Contributor

@huyaoyu huyaoyu left a comment

Choose a reason for hiding this comment

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

Some simple comments.

Need a quick call for discussion.

Thank you!

examples/sparse/bal_loader.py Show resolved Hide resolved
examples/sparse/ba_dense.py Outdated Show resolved Hide resolved
examples/sparse/ba_dense.py Outdated Show resolved Hide resolved
examples/sparse/bal_loader.py Outdated Show resolved Hide resolved
examples/sparse/ba_dense.py Show resolved Hide resolved
examples/sparse/bal_utils.py Show resolved Hide resolved
Returns:
Per-pixel reprojection error. The shape is (..., N).
"""
# convert to camera coordinates
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a quick meeting to discuss how to do proper comments to this function.

I have a hard time understanding the magic dimension numbers used in this function. I think future users might try to take a look at how projection errors are implemented and they might run into the same problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

After a discussion, Yaoyu and I reached the conclusion that we should integrate the image undistortion function into pypose's geometry module, and then simply call pypose native functions here. This will avoid exposing the complexity of raw dimension manipulations to the users and also strengthen pypose's API library.

A refactor is on the way.

@hxu296
Copy link
Member Author

hxu296 commented Jul 12, 2023

Hello! Thanks for your review. I am up to a call after the monthly developer meeting through Slack or Zoom. My email is [email protected].

Update: we had a nice in person discussion

@zitongzhan zitongzhan changed the base branch from SparseTensorBlock to sparse/pr September 28, 2023 21:08
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