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

Dsf-gtsfm-refactor #1312

Merged
merged 34 commits into from
Oct 24, 2022
Merged

Dsf-gtsfm-refactor #1312

merged 34 commits into from
Oct 24, 2022

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Oct 23, 2022

More minimal C++ port of John's DSF track generation code.

  1. Does not have NamedSfmMeasurement
  2. Makes SfmTrack2d a gtsam type, that SfmTrack inherits from
  3. Puts code in cpp file, broken up in easier to grok functions

senselessdev1 and others added 30 commits July 15, 2022 23:32
gtsam/sfm/SfmTrack.h Outdated Show resolved Hide resolved
Copy link
Contributor

@johnwlambert johnwlambert left a comment

Choose a reason for hiding this comment

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

Thank you very much Frank! Much cleaner. LGTM

gtsam/sfm/SfmTrack.h Show resolved Hide resolved
gtsam/sfm/sfm.i Outdated Show resolved Hide resolved
gtsam/sfm/sfm.i Outdated Show resolved Hide resolved
gtsam/sfm/sfm.i Outdated Show resolved Hide resolved
gtsam/sfm/sfm.i Outdated Show resolved Hide resolved
gtsam/sfm/DsfTrackGenerator.h Outdated Show resolved Hide resolved
gtsam/sfm/DsfTrackGenerator.cpp Outdated Show resolved Hide resolved
@dellaert
Copy link
Member Author

@johnwlambert added a vectorized interface for extra speed. If the DSF is still a bottleneck, we could also try a DSFBase with 10000000*i1 + k1 as key. DSFBase uses vector rather than map and can be significantly faster.

@dellaert
Copy link
Member Author

PS, those two vectorized methods in SfmTrack2d were almost completely written by Github CoPilot. Pretty amazing :-)

@dellaert dellaert merged commit 05d4d91 into develop Oct 24, 2022
@dellaert dellaert deleted the dsf-gtsfm-refactor branch October 24, 2022 01:28
@varunagrawal
Copy link
Collaborator

Was this tested with the Matlab wrapper? 🙁 I seem to be having compilation issues here...

@akshay-krishnan
Copy link
Contributor

akshay-krishnan commented Oct 24, 2022

Was this tested with the Matlab wrapper? 🙁 I seem to be having compilation issues here...

The CI does not run matlab wrapper tests? (stupid question, probably)

@varunagrawal
Copy link
Collaborator

Nope. Matlab needs a license so we can't install it on the CI.

size_t size() const;
bool empty() const;
void clear();
gtsam::gtsfm::CorrespondenceIndices at(const pair<size_t, size_t>& keypair) const;
Copy link
Collaborator

@varunagrawal varunagrawal Oct 24, 2022

Choose a reason for hiding this comment

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

This line breaks the Matlab wrapper because std::map doesn't have an at method that takes a std::pair as its argument, rather it takes the key type which is gtsam::IndexPair.

Docs

@dellaert there is no unit test for this.
@johnwlambert @akshay-krishnan reviewers need to do a better job. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

My wrapper knowledge is very limited, sorry for missing that!

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries man. Just something to keep in mind going forward. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

what would the fix be? just changing this to const IndexPair& keypair?
if we do that, can we do .at((i, j)) in. python? or will we need to construct IndexPair(i, j) ?

@varunagrawal
Copy link
Collaborator

@johnwlambert @akshay-krishnan can I expect a fix from one of you two soon? @mattking-smith and I need the Matlab wrapper for a paper he's working on and needs to submit soon.

@akshay-krishnan
Copy link
Contributor

@johnwlambert @akshay-krishnan can I expect a fix from one of you two soon? @mattking-smith and I need the Matlab wrapper for a paper he's working on and needs to submit soon.

how urgent is it? I would be able to send a fix tonight, and I doubt @johnwlambert will be able to get to this sooner.

@johnwlambert
Copy link
Contributor

Unfortunately I don’t own a Matlab license so I won’t be able to test it out. Could we set up an octave CI? Would the octave install instructions match the matlab ones 1:1? Alternatively, a matlab license key could be stored in the gtsam GitHub Actions secrets file to install it during a CI run.

@varunagrawal
Copy link
Collaborator

@johnwlambert @akshay-krishnan can I expect a fix from one of you two soon? @mattking-smith and I need the Matlab wrapper for a paper he's working on and needs to submit soon.

how urgent is it? I would be able to send a fix tonight, and I doubt @johnwlambert will be able to get to this sooner.

I would say it is pretty urgent. If you make the PR, I can review it and test it out on my machine which has the Matlab wrappers built.

@johnwlambert Octave won't work since the bindings don't match up exactly. Uploading a Matlab license might be a legal hassle. I don't have the cycles for this currently, so if you want to take this up, please feel free.

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.

4 participants