Skip to content

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Dec 22, 2017

PR Summary

This sometimes produces something just slightly different from 0 compared to x86(_64). I don't really know how to write a test for this as it only seems to be triggered on alternate arches, but a few test images got 'corrected', so I guess that's good enough.

Fixes #7797.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • [N/A] Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogic QuLogic added this to the v2.1.2 milestone Dec 22, 2017
@@ -391,7 +391,8 @@ namespace agg
vc.remove_all();

double cp = cross_product(v0.x, v0.y, v1.x, v1.y, v2.x, v2.y);
if(cp != 0 && (cp > 0) == (m_width > 0))
Copy link
Member

Choose a reason for hiding this comment

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

What is the operator prescience in c++? Would

if(cp != 0 && ((cp > 0) == (m_width > 0)))

also work?

Copy link
Member Author

Choose a reason for hiding this comment

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

cp is non-zero on the other arches; it doesn't really have to do with precedence. The switch to two separate comparisons is so that -eps < cp < eps is considered 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

See this comment from the issue.

@QuLogic
Copy link
Member Author

QuLogic commented Dec 22, 2017

There's another call to cross_product with a comparison to 0 in that file; I'm not sure if that should be made fuzzy as well.

@QuLogic
Copy link
Member Author

QuLogic commented Dec 23, 2017

Unfortunately, it looks like Windows is producing test_streamplot/streamplot_masks_and_nans.png very similar to the previous version (rms 0.0025 diff from that one) though the other images were okay. That last commit to change the other cases doesn't seem to have made any difference anywhere.

@QuLogic QuLogic force-pushed the cross-platform-barbs branch from 4720fb3 to 4aaf1ef Compare December 24, 2017 05:20
@QuLogic QuLogic force-pushed the cross-platform-barbs branch from 4aaf1ef to 6c5f213 Compare January 10, 2018 09:28
This sometimes produces something just slightly different from 0
compared to x86(_64).
The problem arises in some other location that is more difficult to
determine than the previous one.
@QuLogic QuLogic force-pushed the cross-platform-barbs branch from 6c5f213 to 7216b8c Compare January 11, 2018 06:56
@QuLogic QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 12, 2018
@QuLogic
Copy link
Member Author

QuLogic commented Jan 12, 2018

Would like this in so we don't need too many downstream patches.

@@ -391,7 +391,8 @@ namespace agg
vc.remove_all();

double cp = cross_product(v0.x, v0.y, v1.x, v1.y, v2.x, v2.y);
if(cp != 0 && (cp > 0) == (m_width > 0))
if ((cp > agg::vertex_dist_epsilon && m_width > 0) ||
(cp < -agg::vertex_dist_epsilon && m_width < 0))
Copy link
Member

Choose a reason for hiding this comment

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

Not claiming to understand what you are doing here....

Copy link
Member Author

Choose a reason for hiding this comment

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

Simply treating -eps < 0 < eps as 0 instead of strictly 0.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

Seems logical and worth a try, though I'm puzzled that apparently it required raising the tolerance on Windows.

@QuLogic
Copy link
Member Author

QuLogic commented Jan 13, 2018

Yea, I'm not happy about raising the tolerance for Windows either. Maybe I should try an increased epsilon instead? Edit: that just causes more failures on x86_64 Linux, so maybe not.

@tacaswell tacaswell merged commit e8c57c5 into matplotlib:master Jan 15, 2018
@QuLogic QuLogic deleted the cross-platform-barbs branch January 15, 2018 21:46
tacaswell added a commit that referenced this pull request Jan 15, 2018
The test image was re-generated and the tolerance removed in #10077.

Re-store the tolerance on 2.1.x to gets tests passing, this should not
be merged into master branch.
jklymak added a commit that referenced this pull request Jan 16, 2018
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.

4 participants