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

New-style Errors #1377

Closed
wants to merge 1 commit into from
Closed

New-style Errors #1377

wants to merge 1 commit into from

Conversation

dellaert
Copy link
Member

In an effort to get past the Windows compilation problems on #1375, and, because "friends don't let friends inherit from STL containers", here is an attempt to define Errors as a typedef to FastList<Vector>.

This might still run into the same problem of needing a compare operator for merge on Windows, we'll see in CI...

Thanks @gchenfc for your help in identifying the issue.

@dellaert dellaert requested review from gchenfc and ProfFan January 10, 2023 04:12
Comment on lines +36 to +44
/// Break V into pieces according to its start indices.
GTSAM_EXPORT Errors createErrors(const VectorValues& V);

/** Addition */
GTSAM_EXPORT Errors operator+(const Errors& b) const;
/// Print an Errors instance.
GTSAM_EXPORT void print(const Errors& e, const std::string& s = "Errors");

/** subtraction */
GTSAM_EXPORT Errors operator-(const Errors& b) const;
// Check equality for unit testing.
GTSAM_EXPORT bool equality(const Errors& actual, const Errors& expected,
double tol = 1e-9);
Copy link
Member

Choose a reason for hiding this comment

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

These three functions break backward compatibility for people not using traits, though I'm sure you were already aware.

@gchenfc
Copy link
Member

gchenfc commented Jan 10, 2023

CI failures appear to just be wrapper API change:

gtsam/gtsam/linear/linear.i

Lines 632 to 637 in 16136ea

Errors(const gtsam::VectorValues& V);
//Testable
void print(string s = "Errors");
bool equals(const gtsam::Errors& expected, double tol) const;
};

unclear how to best handle this.

(Linux CI failure appears to just be github hiccup)

@dellaert
Copy link
Member Author

I have killed the wrapper in #1375 , it was not used and very unlikely to be used by anyone outside. I will close this PR as I incorporated changes in #1375 the moment windows succeeded :-) thanks again for your help!

@dellaert dellaert closed this Jan 10, 2023
@dellaert dellaert deleted the fix/Errors branch January 10, 2023 17:38
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.

2 participants