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

Expose GNC params to python #1239

Merged
merged 24 commits into from
Jul 28, 2022
Merged

Expose GNC params to python #1239

merged 24 commits into from
Jul 28, 2022

Conversation

amadoantonini
Copy link
Contributor

No description provided.

@varunagrawal
Copy link
Collaborator

@amadoantonini thanks for the PR. Can you please add some python unit tests to verify everything works as expected?

@amadoantonini
Copy link
Contributor Author

@varunagrawal added some tests, let me know if those are ok.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Awesome stuff @amadoantonini

@varunagrawal
Copy link
Collaborator

@amadoantonini can you please resolve the CI failures?

@amadoantonini
Copy link
Contributor Author

I can try. Any ideas as to why those seemingly unrelated tests fail?

@ProfFan
Copy link
Collaborator

ProfFan commented Jul 14, 2022

I think this is the problem:

Traceback (most recent call last):
[1981](https://github.com/borglab/gtsam/runs/7333219428?check_suite_focus=true#step:7:1982)
  File "/Users/runner/work/gtsam/gtsam/python/gtsam/tests/test_NonlinearOptimizer.py", line 128, in test_gnc_params
[1982](https://github.com/borglab/gtsam/runs/7333219428?check_suite_focus=true#step:7:1983)
    params.setKnownInliers(inl)
[1983](https://github.com/borglab/gtsam/runs/7333219428?check_suite_focus=true#step:7:1984)
TypeError: setKnownInliers(): incompatible function arguments. The following argument types are supported:
[1984](https://github.com/borglab/gtsam/runs/7333219428?check_suite_focus=true#step:7:1985)
    1. (self: gtsam.gtsam.GncLMParams, knownIn: std::__1::vector<unsigned long, std::__1::allocator<unsigned long> >) -> None

std::__1::vector<unsigned long, std::__1::allocator<unsigned long> > need to have a type alias and wrapped. This is also essential for the MATLAB wrapper to work.

@ProfFan
Copy link
Collaborator

ProfFan commented Jul 14, 2022

P.S. I have 0 idea why the automatic binding code cause so many hard-to-debug issues. I encountered an infinity loop issue that is caused by stl.h when wrapping the hybrid code, took me a week to debug. Should we try upgrade the pybind11 lib and see if problems disappear? @varunagrawal

@varunagrawal
Copy link
Collaborator

This looks like the Jacobians are not being copied back to the C++ side of things. I guess we have to look at how the opacity is being set.

This is also a good time to revisit my PR on using the stl bindings fully.

@varunagrawal
Copy link
Collaborator

@amadoantonini the issue seems to be tangential to your fix. @ProfFan and I are working on figuring this out, so we thank you for your patience!

@varunagrawal
Copy link
Collaborator

@amadoantonini I opened a PR to fix your issue openspacelabs#1
Can you please merge it in? It should automatically reflect here.

@varunagrawal
Copy link
Collaborator

Ahhh I'm going to have to look into this more.

@amadoantonini
Copy link
Contributor Author

Thanks @varunagrawal! Let me know if I can help.

@varunagrawal
Copy link
Collaborator

@amadoantonini can you please merge in the latest develop? We made some CI changes to fix issues we've been seeing lately (this PR included).

@amadoantonini
Copy link
Contributor Author

That seems to have fixed it on my side!

@varunagrawal
Copy link
Collaborator

@amadoantonini gentle reminder about openspacelabs#2

@varunagrawal
Copy link
Collaborator

@amadoantonini can you please merge in the latest develop? Github Actions is being funky.

@amadoantonini
Copy link
Contributor Author

@varunagrawal I had to resolve a conflict. I think I did the right thing based on the order of your changes. Please verify.

@varunagrawal
Copy link
Collaborator

Yup looks good to me.

@varunagrawal
Copy link
Collaborator

@amadoantonini please merge this openspacelabs#3 and please allow maintainers to make edits so I can push here directly.

@varunagrawal varunagrawal merged commit 8051e74 into borglab:develop Jul 28, 2022
@varunagrawal
Copy link
Collaborator

Yay finally merged!

@amadoantonini
Copy link
Contributor Author

Thanks for all the support @varunagrawal !

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.

3 participants