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

Various Fixes and Updates #1723

Merged
merged 7 commits into from
Feb 28, 2024
Merged

Various Fixes and Updates #1723

merged 7 commits into from
Feb 28, 2024

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Feb 22, 2024

  1. Undo Python CI changes since the intended effects are not being purported. The Python-TBB CI needs more investigation to debug what the underlying issue is rather than making random guesses.
  2. Fix boost collision issue for save, load, etc with boost/serialization/optional.hpp.
  3. Wrapped the BarometricFactor as required in Custom Factor in MATLAB #1722.

I ran this locally on my machine Matlab and it works great!

@varunagrawal varunagrawal added the matlab Related to MATLAB wrapper label Feb 22, 2024
@varunagrawal varunagrawal requested a review from ProfFan February 22, 2024 20:09
@varunagrawal varunagrawal self-assigned this Feb 22, 2024
@talregev
Copy link
Contributor

It not fair that you cancel my PR, but you use my changes. Be fair.

@varunagrawal
Copy link
Collaborator Author

@talregev your PR didn't clearly state what the issue you were trying to solve was. I encountered the issue myself this week and recalled your fix.
If you want your PRs to be accepted and merged, you need to better communicate what the issue is and how you're solving it. As a reviewer, I have limited time and the onus is on you to write a good PR.

@talregev
Copy link
Contributor

Even you think I wasn't communicate enough, and then you recall my fix, Do you think is a reason to steal it like that?

I will put my description here of my PR that the community will judge if it was communicate enough.

Bug fix for macos: Support new version of boost along side old version. #1721
In macos vm, recently we get a newer version of boost (1.84).
They already implemented in the new version the function that gtsam implemented.
So I am not sure the correct version of boost that it begin to happen, so I put version 1.80.
We can tune the version later (when we have another issue similar to that in different boost version).
@varunagrawal @ProfFan
Thank you for your time and effort for review this PR.

@varunagrawal
Copy link
Collaborator Author

Sounds good.

I'm re-reading your comment and it clearly doesn't mention what issue your PR is trying to fix. There is some vague mention of the MacOS VM but nothing about the fact that the latest boost version is causing a name collision with std::optional.

I looked up the error and the suggested fix was similar to yours which is why I recalled your PR. There was no ill intention and I didn't have the time to reopen your PR and go through the review back and forth, hence why I added it to this PR.

@borglab borglab locked and limited conversation to collaborators Feb 24, 2024
@borglab borglab unlocked this conversation Feb 24, 2024
@varunagrawal varunagrawal changed the title Wrap barometric factor Various Changes Feb 26, 2024
@varunagrawal
Copy link
Collaborator Author

The fix on macOS doesn't seem to generalize to Ubuntu. I'll look into it later today.

@varunagrawal varunagrawal changed the title Various Changes Various Fixes and Updates Feb 26, 2024
@varunagrawal varunagrawal linked an issue Feb 26, 2024 that may be closed by this pull request
@varunagrawal
Copy link
Collaborator Author

@ProfFan FYI this PR has a better analysis of the issue that #1721 is trying to fix and thus has the better solution.

Copy link
Collaborator

@ProfFan ProfFan left a comment

Choose a reason for hiding this comment

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

Sans one comment

@varunagrawal varunagrawal merged commit 0149005 into develop Feb 28, 2024
31 checks passed
@varunagrawal varunagrawal deleted the wrap-barometricfactor branch February 28, 2024 15:37
@varunagrawal varunagrawal restored the wrap-barometricfactor branch February 28, 2024 15:37
@varunagrawal varunagrawal deleted the wrap-barometricfactor branch February 28, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
matlab Related to MATLAB wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Factor in MATLAB
3 participants