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

Consistent Rot3 printing #358

Merged
merged 13 commits into from
Jul 28, 2020
Merged

Consistent Rot3 printing #358

merged 13 commits into from
Jul 28, 2020

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Jun 18, 2020

This PR unifies the printing functionality of Rot3 to use the Eigen::IOFormat formatting, thus ensuring consistency.

The aim would be to help the user visually identify more easily what the rotation matrices are, as well as copy them into another language such as python.

I've also

  1. updated the printing of Pose3, NavState and Rot3 to provide default spacing when using the custom prefix string
  2. removed extra newlines by removing newlines in the << overload
  3. removed endl in << so as to avoid unnecessary buffer flushes.

For the below snippet,

Pose3 p;
cout << p << endl;
p.print();

the previous format looked like this:

# result of cout
|1, 0, 0|
|0, 1, 0|
|0, 0, 1|

[0, 0, 0]';

# result of .print()
R:
[
	1, 0, 0;
 	0, 1, 0;
 	0, 0, 1
]
[0, 0, 0]';

The new format would give the output:

# result of cout
R: [
	1, 0, 0;
	0, 1, 0;
	0, 0, 1
]
t: [0, 0, 0]'
# result of .print()
R: [
	1, 0, 0;
	0, 1, 0;
	0, 0, 1
]
t: [0, 0, 0]'

This change is Reviewable

@varunagrawal varunagrawal added enhancement Improvement to GTSAM feature New proposed feature design Design choices labels Jun 18, 2020
@varunagrawal varunagrawal self-assigned this Jun 18, 2020
@varunagrawal varunagrawal requested review from dellaert and ProfFan June 18, 2020 16:15
@varunagrawal varunagrawal mentioned this pull request Jun 18, 2020
26 tasks
@varunagrawal
Copy link
Collaborator Author

varunagrawal commented Jun 18, 2020

Additionally, this is how NavState now prints

R: [
	1, 0, 0;
	0, 1, 0;
	0, 0, 1
]
p: [0, 0, 0]'
v: [0, 0, 0]'

@dellaert
Copy link
Member

Ask other people for reviews ?

@dellaert
Copy link
Member

Ask other people for reviews ?

Dude. I don’t mean ask everyone at once :-)

Copy link
Member

@jlblancoc jlblancoc left a comment

Choose a reason for hiding this comment

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

LGTM! Only, see a couple of comments above.

gtsam/base/Matrix.h Show resolved Hide resolved
gtsam/geometry/Rot3.cpp Outdated Show resolved Hide resolved
gtsam/navigation/NavState.cpp Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator Author

@dellaert sorry about that. I figured casting a wide net would yield faster results. 😅

@varunagrawal varunagrawal requested review from jlblancoc and removed request for thduynguyen, mikesheffler and akshay-krishnan June 19, 2020 21:06
@varunagrawal varunagrawal removed the request for review from gchenfc June 19, 2020 21:06
@varunagrawal varunagrawal added this to the GTSAM 4.1 milestone Jul 5, 2020
@dellaert dellaert modified the milestones: GTSAM 4.1, GTSAM 4.1.1 Jul 14, 2020
@varunagrawal
Copy link
Collaborator Author

@dellaert @jlblancoc now that 4.0.3 is out, I would really like to see this PR merged in. 🙂

Copy link
Member

@jlblancoc jlblancoc left a comment

Choose a reason for hiding this comment

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

It might seem I'm obsessed with this tiny performance issue but... again... std::endl -> \n ? :-)

@varunagrawal
Copy link
Collaborator Author

Whoops. Yeah the other PR is based off this one hence you're seeing the issue repeat itself. Give me a few quick minutes to update this.

@varunagrawal
Copy link
Collaborator Author

So I am a bit confused about the endl to \n conversion left here. From my understanding, we want to do a buffer flush when calling print() and that is the only place where we use endl. :-)

@jlblancoc
Copy link
Member

So I am a bit confused about the endl to \n conversion left here. From my understanding, we want to do a buffer flush when calling print() and that is the only place where we use endl. :-)

Hmm... I'm thinking of, say, we call print() on a Values or an entire FactorGraph. One flush at the end is ok... but 1000s of them, after printing (recursively) each value/matrix/vector/factor/etc. seems not necessary :-)

Still, it's probably a minor point, so feel free of leaving it as you think it's better.

@varunagrawal
Copy link
Collaborator Author

I completely agree with your point. The issue is that the Values/FactorGraph print method just calls the internal print method of the various values/factors, so we need to add the endl over there. I think that should be left to a future PR?

@jlblancoc
Copy link
Member

Ok. I don't want to stop you doing anything more productive!
Merge it when you want to.

@jlblancoc jlblancoc self-requested a review July 24, 2020 14:05
@varunagrawal
Copy link
Collaborator Author

Merging because Travis keeps timing out and we will be moving to Github Actions.

@varunagrawal varunagrawal merged commit c985701 into develop Jul 28, 2020
@varunagrawal varunagrawal deleted the feature/rot-print branch July 28, 2020 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design choices enhancement Improvement to GTSAM feature New proposed feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants