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

Use fuzzy comparison for stroke join determination. #10077

Merged
merged 2 commits into from Jan 15, 2018

Conversation

Projects
None yet
4 participants
@QuLogic
Copy link
Member

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))

This comment has been minimized.

Copy link
@tacaswell

tacaswell Dec 22, 2017

Member

What is the operator prescience in c++? Would

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

also work?

This comment has been minimized.

Copy link
@QuLogic

QuLogic Dec 22, 2017

Author Member

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.

This comment has been minimized.

Copy link
@QuLogic

QuLogic Dec 22, 2017

Author Member

See this comment from the issue.

@QuLogic

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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 QuLogic:cross-platform-barbs branch from 4720fb3 to 4aaf1ef Dec 24, 2017

@QuLogic QuLogic force-pushed the QuLogic:cross-platform-barbs branch from 4aaf1ef to 6c5f213 Jan 10, 2018

QuLogic added some commits Oct 15, 2017

Use fuzzy comparison for stroke join determination.
This sometimes produces something just slightly different from 0
compared to x86(_64).
TST: Increase some tolerances for Windows.
The problem arises in some other location that is more difficult to
determine than the previous one.

@QuLogic QuLogic force-pushed the QuLogic:cross-platform-barbs branch from 6c5f213 to 7216b8c Jan 11, 2018

@QuLogic

This comment has been minimized.

Copy link
Member Author

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))

This comment has been minimized.

Copy link
@jklymak

jklymak Jan 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@QuLogic

QuLogic Jan 12, 2018

Author Member

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

@efiring
Copy link
Member

left a comment

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

@QuLogic

This comment has been minimized.

Copy link
Member Author

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

8 checks passed

ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project/library 64.22% (target 50%)
Details
codecov/project/tests 98.85% (+<.01%) compared to c96014f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

meeseeksdev bot pushed a commit that referenced this pull request Jan 15, 2018

@QuLogic QuLogic deleted the QuLogic:cross-platform-barbs branch Jan 15, 2018

tacaswell added a commit that referenced this pull request Jan 15, 2018

TST: add tolerance for tests to pass on 2.1.x branch
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
You can’t perform that action at this time.