Skip to content

Astronet loader - #258

Merged
travisdriver merged 31 commits into
masterfrom
astronet-loader
Sep 10, 2021
Merged

Astronet loader#258
travisdriver merged 31 commits into
masterfrom
astronet-loader

Conversation

@travisdriver

Copy link
Copy Markdown
Collaborator

No description provided.

@travisdriver

Copy link
Copy Markdown
Collaborator Author

2011212_opnav_022-gtsfm-scene-02092021

Current results on 2011205_opnav_022 segment of the Dawn mission to 4 Vesta. Plenty of room for improvement :)

Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread tests/loader/test_astronet_loader.py Outdated
Comment thread tests/loader/test_astronet_loader.py Outdated

TEST_DATA_ROOT = Path(__file__).resolve().parent.parent / "data"
TEST_POINT3D_IDS = [7, 2560, 3063, 3252, 3424, 3534, 3653, 3786, 3920, 4062, 4420, 4698, 3311, 3963, 325]
TEST_SFMTRACKS_INDICES = [0, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1000, 1100, 1200, 1300, 1400]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could you add an inline comment explaining where these come from and how they were selected?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Explanation added.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If these have a 1:1 correspondences, it might be better to use a list of tuples instead of 2 lists. Ive found that to cause fewer bugs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

They're used individually as well, so I thought it would be clearer to split them up.

Comment thread gtsfm/runner/run_scene_optimizer_astronet.py Outdated
Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread gtsfm/loader/astronet_loader.py
Comment thread environment_linux.yml Outdated
@akshay-krishnan

Copy link
Copy Markdown
Collaborator

Please provide a PR description. I'll just let John and Ayush review this, as they are familiar with the loaders. :)

@akshay-krishnan
akshay-krishnan removed their request for review September 3, 2021 01:42
@johnwlambert

Copy link
Copy Markdown
Collaborator

Is the figure you posted

Current results on 2011205_opnav_022 segment of the Dawn mission to 4 Vesta.

from the deep front end?

@travisdriver

Copy link
Copy Markdown
Collaborator Author

Is the figure you posted

Current results on 2011205_opnav_022 segment of the Dawn mission to 4 Vesta.

from the deep front end?

Yes it is

@johnwlambert

Copy link
Copy Markdown
Collaborator

Is the figure you posted

Current results on 2011205_opnav_022 segment of the Dawn mission to 4 Vesta.

from the deep front end?

Yes it is

cool, indeed lots of room for improvement :-)

Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread gtsfm/loader/astronet_loader.py Outdated
@johnwlambert

Copy link
Copy Markdown
Collaborator

Thanks for all the updates, Travis. Can we put the Astronet test data under an astronet subdir?

instead of tests/data/test_2011212_opnav_022/images/00000004.png
i would prefer tests/data/astronet/test_2011212_opnav_022/images/00000004.png
so people can know what dataset opnav refers to

Comment thread thirdparty/colmap/scripts/python/read_write_model.py Outdated
Comment thread thirdparty/colmap/scripts/python/read_write_model.py Outdated
Comment thread thirdparty/colmap/scripts/python/read_write_model.py Outdated
Comment thread thirdparty/colmap/scripts/python/read_write_model.py
Comment thread thirdparty/colmap/scripts/python/read_write_model.py
@johnwlambert

Copy link
Copy Markdown
Collaborator

Looks quite good. Thanks Travis. @ayushbaid could you take another look when you get a chance since you are the most familiar with the BaseLoader class design?

Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread tests/loader/test_astronet_loader.py Outdated

from gtsfm.common.image import Image
from gtsfm.loader.astronet_loader import AstroNetLoader
from thirdparty.colmap.scripts.python.read_write_model import read_model

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same note here about importing an individual function vs. Google style

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread gtsfm/loader/astronet_loader.py Outdated
logger = logger_utils.get_logger()


class AstroNetLoader(LoaderBase):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should the class be called AstronetLoader, considering the context here? The file is also called astronet_loader.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My dataset is called AstroNet, so yeah I think it should be AstronetLoader instead.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Akshay did you mean disambiguating from a CNN architecture/network name here?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah I just meant using a small 'n' instead of 'N' because Astronet is one thing as far as the loader is concerned. Its resolved now.

Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread gtsfm/loader/astronet_loader.py Outdated
Comment thread gtsfm/utils/io.py

TEST_DATA_ROOT = Path(__file__).resolve().parent.parent / "data"
TEST_POINT3D_IDS = [7, 2560, 3063, 3252, 3424, 3534, 3653, 3786, 3920, 4062, 4420, 4698, 3311, 3963, 325]
TEST_SFMTRACKS_INDICES = [0, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1000, 1100, 1200, 1300, 1400]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If these have a 1:1 correspondences, it might be better to use a list of tuples instead of 2 lists. Ive found that to cause fewer bugs.

Comment on lines +49 to +50
# assert self.loader._gt_scene_trimesh is not None
# assert self.loader._gt_scene_trimesh.vertices;shape[0] == 5002

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

remove if unnecessary?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I will be uncommenting this (and some other tests) in the next PR when I add trimesh. It would be useful to be able to keep them here so I don't have to rewrite them.

Comment thread tests/loader/test_astronet_loader.py
Comment thread gtsfm/loader/astronet_loader.py
@travisdriver
travisdriver merged commit 9b2dbb0 into master Sep 10, 2021
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.

5 participants