Skip to content

Use fuzzy comparison for stroke join determination. #10077

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 15, 2018
Merged

Use fuzzy comparison for stroke join determination. #10077

merged 2 commits into from
Jan 15, 2018

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
tacaswell
tacaswell reviewed Dec 22, 2017
View reviewed changes
extern/agg24-svn/include/agg_math_stroke.h
@@ -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 •
edited
Loading

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
@QuLogic
Use fuzzy comparison for stroke join determination.
61c1de0 
This sometimes produces something just slightly different from 0
compared to x86(_64).
@QuLogic
TST: Increase some tolerances for Windows.
7216b8c 
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.

jklymak
jklymak approved these changes Jan 12, 2018
View reviewed changes
extern/agg24-svn/include/agg_math_stroke.h
@@ -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.

efiring
efiring approved these changes Jan 12, 2018
View reviewed changes
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 •
edited
Loading

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
lumberbot-app bot pushed a commit that referenced this pull request Jan 15, 2018
@tacaswell
Backport PR #10077: Use fuzzy comparison for stroke join determination.
8a3569d
@lumberbot-app lumberbot-app bot mentioned this pull request Jan 15, 2018
Merged
@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
@tacaswell
TST: add tolerance for tests to pass on 2.1.x branch
20ac580 
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
@jklymak
Merge pull request #10253 from matplotlib/auto-backport-of-pr-10077
d388d2f 
Backport PR #10077 on branch v2.1.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@efiring efiring efiring approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v2.1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@QuLogic @efiring @tacaswell @jklymak