Dsf-gtsfm-refactor #1312
Dsf-gtsfm-refactor #1312
Conversation
…tsfm-to-gtsam-port
…tsfm-to-gtsam-port
…tsfm-to-gtsam-port
|
@johnwlambert added a vectorized interface for extra speed. If the DSF is still a bottleneck, we could also try a DSFBase with |
|
PS, those two vectorized methods in SfmTrack2d were almost completely written by Github CoPilot. Pretty amazing :-) |
|
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) |
|
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; |
There was a problem hiding this comment.
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.
@dellaert there is no unit test for this.
@johnwlambert @akshay-krishnan reviewers need to do a better job. 🙂
There was a problem hiding this comment.
My wrapper knowledge is very limited, sorry for missing that!
There was a problem hiding this comment.
No worries man. Just something to keep in mind going forward. 🙂
There was a problem hiding this comment.
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) ?
|
@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. |
|
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. |
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. |
More minimal C++ port of John's DSF track generation code.