Skip to content

FIX: do not skip isolated points in vector outputs #8297

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

Merged
merged 1 commit into from
Mar 15, 2017

Conversation

tacaswell
Copy link
Member

Fixed bug in path interaction code where multiple moves in a
row (which correspond to isolated points surrounded by nans after they
have been filtered from the path) would be discarded which resulted in
points not being plotted.

closes #7293

@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Mar 14, 2017
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Mar 14, 2017
@@ -117,6 +117,19 @@ def test_marker_paths_pdf():
plt.ylim(-1, 7)


@image_comparison(baseline_images=['nan_path'], style='default',
remove_text=True,extensions=['pdf', 'svg', 'eps', 'png'])
Copy link
Member

Choose a reason for hiding this comment

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

Space after ,.

@@ -343,6 +343,7 @@ class PathClipper : public EmbeddedQueue<3>
unsigned vertex(double *x, double *y)
{
unsigned code;
bool emit_moveto = false;
Copy link
Member

Choose a reason for hiding this comment

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

Our C/C++ code uses spaces as well, not tabs.

Copy link
Member Author

Choose a reason for hiding this comment

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

gah! emacs has betrayed me!

// seen at least one command ?
// if so, shove it in the queue if in clip box
if (m_moveto && m_has_init &&
m_lastX >= m_cliprect.x1 &&
Copy link
Member

Choose a reason for hiding this comment

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

I prefer reversing the arguments here so that it looks similar to Python's chained form.

Copy link
Member Author

Choose a reason for hiding this comment

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

This matches the block further down, would rather stay consistent with that.

m_lastX <= m_cliprect.x2 &&
m_lastY >= m_cliprect.y1 &&
m_lastY <= m_cliprect.y2) {
// push the last moveto onto the queue
Copy link
Member

Choose a reason for hiding this comment

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

moveto -> move to (or maybe the reverse for the other places would be clearer.)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this comment is kind of redundant with the one above the if.

m_initX = m_lastX = *x;
m_initY = m_lastY = *y;
m_has_init = true;
m_moveto = true;
// if the last command was move to exit the loop to emit the code
Copy link
Member

Choose a reason for hiding this comment

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

I mean, this is pretty clearly what it does already, so it's kind of a redundant comment. Would be better as a why instead of a what.

@tacaswell
Copy link
Member Author

Have not bisected, but looking at the blame I suspect this was broken by #5911 so broken from 1.5.2 on.

@tacaswell tacaswell force-pushed the fix_missing_marker branch from bb0d1a9 to 27d4596 Compare March 14, 2017 21:08
Fixed bug in path interaction code where multiple moves in a
row (which correspond to isolated points surrounded by nans after they
have been filtered from the path) would be discarded which resulted in
points not being plotted.

closes matplotlib#7293
@tacaswell tacaswell force-pushed the fix_missing_marker branch from 27d4596 to 5f54543 Compare March 14, 2017 21:09
m_lastX <= m_cliprect.x2 &&
m_lastY >= m_cliprect.y1 &&
m_lastY <= m_cliprect.y2) {
// push the last moveto onto the queue
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is still redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote this as comments to my self while understanding this. I do not see a problem with leaving them.

@NelleV NelleV merged commit 371b8b1 into matplotlib:v2.0.x Mar 15, 2017
@NelleV
Copy link
Member

NelleV commented Mar 15, 2017

Thanks @tacaswell !

@tacaswell tacaswell deleted the fix_missing_marker branch March 15, 2017 18:20
@tacaswell
Copy link
Member Author

tacaswell commented Mar 15, 2017

This PR was about 2 LoC / hr 😆 (of changes to the source, not the surrounding tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants