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

Fix plot details, correcting xlabels positions and cleaning the graph #1658

Merged
merged 5 commits into from
Sep 12, 2019
Merged

Fix plot details, correcting xlabels positions and cleaning the graph #1658

merged 5 commits into from
Sep 12, 2019

Conversation

KaikeWesleyReis
Copy link

Description

Correcting minor issues in plot results from moveit_benchmark_statistics.py. This changes was made to increase the graphical potencial from this tool to papers plots (This solution first came for a publication that I came with, using moveit benchmarking).

Checklist

  • Change xtick label rotation from 30 to 45, fixing the issue that the labels was not syncronyzed with the boxplot/barplot.
  • Solve the labels names for OMPL planners that by default appears with 'kConfigDefault* at the end (some websites recommend to put this string in the yaml file definition from moveit!). This solutions replace this part of the string by ''
  • Take out nanSolutions count from the ax.text, making the chart cleaner

Plot Example

Before

a1

After

a2

OBS This is my first contribution to a project in github, any issue or anything please respond this request.

Best regards,

@welcome
Copy link

welcome bot commented Sep 3, 2019

Thanks for helping in improving MoveIt

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Hi there,

thank you for taking the time to file a pull-request for this. Could you please address my feedback?

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Looks good to me, I approve.

For scientific publications you should setup your own plots (fitting to your paper), but it does look a bit nicer.

@v4hn v4hn added the awaits 2nd review one maintainer approved this request label Sep 10, 2019
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

@KaikeWesleyReis, looks like your fixup commit messed up with indentation. I pushed another fixup commit - please have a look.
I also re-enabled displaying the number of failed attempts. While you might not want to see those figures in your plot, they might be important for others.
I don't quite understand, why your after image has so much less bars than your before image?
If you changed the benchmark config, the plots become not comparable. Please update the plots.
Finally, I don't really like the change of rotation from 30 to 45 degrees. Less rotation is usually easier to read. However, I definitely would appreciate if you could right-align the labels to the bars. Now, they are centered on the bars, which is confusing when there are many bars and labels.

@rhaschke rhaschke removed the awaits 2nd review one maintainer approved this request label Sep 11, 2019
@KaikeWesleyReis
Copy link
Author

KaikeWesleyReis commented Sep 11, 2019

@rhaschke

@KaikeWesleyReis, looks like your fixup commit messed up with indentation. I pushed another fixup commit - please have a look.

Well, sorry for this part. I reviewed my code and it's indentation in my remote, I will do another commit and if continues this problem, please say how can I get your commit to my fork.

I also re-enabled displaying the number of failed attempts. While you might not want to see those figures in your plot, they might be important for others.

This number appears in all graphs, but there is a specific graph that is exactly this info (solved):

image

So besides the "clean view" as I said before, plotting this value in all graphs would be a over information that I don't think that is necessary.

I don't quite understand, why your after image has so much less bars than your before image?
If you changed the benchmark config, the plots become not comparable. Please update the plots.

Sorry, because I used other db for the second plot. Here is a complete plot (but is not the same data, but with the same planners). PS: You will notice a 90 degrees rotation.
plot

Finally, I don't really like the change of rotation from 30 to 45 degrees. Less rotation is usually easier to read. However, I definitely would appreciate if you could right-align the labels to the bars. Now, they are centered on the bars, which is confusing when there are many bars and labels.

This situation is a bit complex. For example, with a 90 degree rotation you can see that particular bar belongs to that planner. However at 30 degrees it is more complicated to figure out who belongs to whom, because the plot labels shifts to the right (as you can see in the first image from this PR).
I noticed that at 45 degrees, this question persists. My solution would be 90 degrees, but it gets complicated to read (as you could see previously). This could be changed by decreasing the planners name or fontsize to maintain legibility and positioning.

EDIT: Well, I followed what you said and this would be the results (30 degrees, ha='right'). Is this ok to you ?
image

I will wait for further instructions to change my code.

Best regards,

@rhaschke
Copy link
Contributor

I reviewed my code and it's indentation in my remote, I will do another commit and if continues this problem, please say how can I get your commit to my fork.

I already pushed a fixing commit to your fork. You just need to pull it.

I also re-enabled displaying the number of failed attempts. While you might not want to see those figures in your plot, they might be important for others.

This number appears in all graphs, but there is a specific graph that is exactly this info (solved).

I see. In this case, I agree with you. Please feel free to revert to your initial version.

Well, I followed what you said and this would be the results (30 degrees, ha='right'). Is this ok to you?

This looks great! Please push this.

@KaikeWesleyReis
Copy link
Author

@rhaschke I got your commit and made the changes in this new commit. Please, feel free to test and give a feedback.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks a lot for iterating on this. Looks good now.
@v4hn, should we accept this for kinetic? If so, it needs to be forward-ported to melodic and master.

@v4hn v4hn merged commit 353e5de into moveit:kinetic-devel Sep 12, 2019
@welcome
Copy link

welcome bot commented Sep 12, 2019

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

@v4hn
Copy link
Contributor

v4hn commented Sep 12, 2019

should we accept this for kinetic?

You did not reject it in your review so I will assume it's ok with you.
My personal policy is to merge requests in old branches too if that's what users use (and pull-request).

If so, it needs to be forward-ported to melodic and master.

Filed as #1667 and #1668

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.

None yet

3 participants