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

Feature/vector value print #68

Merged
merged 8 commits into from
Jun 16, 2019
Merged

Feature/vector value print #68

merged 8 commits into from
Jun 16, 2019

Conversation

MandyXie
Copy link

@MandyXie MandyXie commented Jun 14, 2019

This change is Reviewable

@MandyXie MandyXie requested a review from dellaert June 14, 2019 03:42
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Mandy, thanks !!! But I don't see how a simple print change should change so many things? Are you sure you have the latest develop? And can you restrict the PR to just the print?
Finally, I suggest you make a unit test. To make that work, make a stream operator << that you call from Print, and do the test with stringstream...

@MandyXie
Copy link
Author

Mandy, thanks !!! But I don't see how a simple print change should change so many things? Are you sure you have the latest develop? And can you restrict the PR to just the print?
Finally, I suggest you make a unit test. To make that work, make a stream operator << that you call from Print, and do the test with stringstream...

Sorry, I will update my pr to the latest develop. However, I have some questions.
First, what do you mean by restricting the PR to just the print?
Second, I tried to overload a stream operator <<, but I don't know how to have it taken two extra arguments which are needed in the print function, const string& str, const KeyFormatter& formatter.

@dellaert
Copy link
Member

If you look at the "changed files" you see that there are differences all over the file. Did you by chance re-format? I'd prefer if that is not done in GTSAM, to make sure the PR and commits are limited to the new functionality.

You are right about the KeyFormatter - that is an issue that we will have to confront when we switch to streams. I think there is a solution with stream manipulators, let me investigate a bit...

@MandyXie MandyXie force-pushed the feature/VectorValuePrint branch from 9fe28e4 to 9dc6b9d Compare June 14, 2019 14:57
Copy link
Author

@MandyXie MandyXie left a comment

Choose a reason for hiding this comment

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

Just a check, will fix the extra changes.

@MandyXie MandyXie force-pushed the feature/VectorValuePrint branch from 9dc6b9d to 7b41007 Compare June 14, 2019 15:15
@dellaert
Copy link
Member

Could you use the DefaultKeyformatter for now in the stream operator and ignore the argument in Print? Then commit with [ci-skip] in the commit message. I will then add the formatting thing - I know how to do it now...

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Getting there !

gtsam/linear/tests/testVectorValues.cpp Outdated Show resolved Hide resolved
gtsam/linear/tests/testVectorValues.cpp Outdated Show resolved Hide resolved
gtsam/linear/VectorValues.h Outdated Show resolved Hide resolved
gtsam/linear/VectorValues.h Outdated Show resolved Hide resolved
gtsam/linear/VectorValues.h Outdated Show resolved Hide resolved
gtsam/linear/VectorValues.cpp Outdated Show resolved Hide resolved
gtsam/linear/VectorValues.cpp Outdated Show resolved Hide resolved
gtsam/linear/VectorValues.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Two small comments remain

gtsam/linear/VectorValues.cpp Outdated Show resolved Hide resolved
gtsam/linear/VectorValues.cpp Show resolved Hide resolved
@dellaert
Copy link
Member

OK, cool, PR #69 is merged which you can use to send the formatter into the stream:

cout << key_formatter(formatter) << blah...

@dellaert dellaert merged commit 5785295 into develop Jun 16, 2019
@dellaert dellaert deleted the feature/VectorValuePrint branch June 16, 2019 23:18
@dellaert dellaert mentioned this pull request Jun 16, 2019
varunagrawal added a commit that referenced this pull request Apr 2, 2021
5ddaff8ba Merge pull request #77 from borglab/fix/template-as-template-arg
0b6f2d92b allow templates as paramters to templates
7f3e242b0 Merge pull request #76 from borglab/fix/cmake-config
0caa79b82 macro to find and configure matlab
522557232 fix GTWRAP_INCLUDE_NAME
78a5d3afa Use CMakePackageConfigHelpers to vastly simplify the package config
76f8b9e5d Merge pull request #75 from borglab/fix/template-args
3b8e8389e remove reference from shared pointers
045393c7b docs and flag renaming
d23d8beae tests
ef96b4bdc don't make template parameters as references
d1e1dc697 Merge pull request #74 from borglab/fix/type-recursion
8202ecf10 minor fixes
5855ea85b support for passing templated types as arguments
150cc0578 Support for templated return types
5c58f8d03 Merge pull request #73 from borglab/fix/types-refactor
c697aa9c8 refactored the basic and custom types to make it cleaner, added more tests
98e2c3fa1 Merge pull request #68 from borglab/fix/cmake
c6d5e786a make config agnostic to install prefix
4d6999f15 Merge pull request #69 from borglab/feature/call-and-index
ccf408804 add support for callable and indexing overloads
8f8e3ec93 add status messages
88566eca4 make WRAP_PYTHON_VERSION an optional argument
01b8368ad Merge pull request #67 from borglab/feature/operator-overloading
522a12801 remove unsupported operators
209346133 update check location for unary operator
39e290f60 fix small typo
faa589bec update DOCS
7ff83cec8 minor fixes
8ce37766f fixed tests
21c477c4d include pybind11/operators
a3534ac5e wrap operator overloads
67c8f2089 instantiate templates for operators
e9dce65d8 use ReturnType for ease in other places and use members in Class
3078aa6db added parser rule for operator overloading

git-subtree-dir: wrap
git-subtree-split: 5ddaff8bab6c05e8a943c94993bf496e13296dd6
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