Use custom RNG for sketch path #5728

Merged
merged 6 commits into from Dec 29, 2015

Conversation

Projects
None yet
4 participants
Owner

mdboom commented Dec 23, 2015

This is an alternative to #5725.

@mdboom mdboom Use custom RNG for sketch path
c1c639a

mdboom added the needs_review label Dec 23, 2015

Owner

jenshnielsen commented Dec 23, 2015

👍 once we can use C++11 I would prefer to use stl random lib but this seems like a suitable solution at the moment

mdboom added some commits Dec 23, 2015

@mdboom mdboom Fix constants
9bce658
@mdboom mdboom fix comment
f656043
Owner

tacaswell commented Dec 23, 2015

looks like xkcd tests are failing (as we changed the random number generator).

Owner

mdboom commented Dec 23, 2015

Ah, of course. I'm not going to try to make it backward compatible, and just update the images :) Honestly, it's probably a good thing -- the C stdlib RNG doesn't guarantee any consistency across compilers anyway, but this should.

Owner

jenshnielsen commented Dec 23, 2015

The xkcd test have always been failing for me on OSX so that seems likely. Would be great if this fixes that too.

Owner

tacaswell commented Dec 23, 2015

👍 to not worrying about back compatibility here. I really really hope no one is relying on the details of the sketch path!

mdboom added some commits Dec 23, 2015

@mdboom mdboom Fix sign of seed b447284
@mdboom mdboom Update test images
de35cc2
Owner

tacaswell commented Dec 23, 2015

appveyor is not failing when it fails, but this is showing up, it used to just fail on pixel errors.

======================================================================
ERROR: matplotlib.tests.test_path.test_xkcd.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python34_64\envs\test-environment\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 54, in failer
    result = f(*args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 232, in do_test
    figure.savefig(actual_fname, **self._savefig_kwarg)
  File "c:\projects\matplotlib\lib\matplotlib\figure.py", line 1698, in savefig
    self.canvas.print_figure(*args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\backend_bases.py", line 2230, in print_figure
    **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\backends\backend_agg.py", line 520, in print_png
    FigureCanvasAgg.draw(self)
  File "c:\projects\matplotlib\lib\matplotlib\backends\backend_agg.py", line 467, in draw
    self.figure.draw(self.renderer)
  File "c:\projects\matplotlib\lib\matplotlib\artist.py", line 62, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\figure.py", line 1230, in draw
    self.patch.draw(renderer)
  File "c:\projects\matplotlib\lib\matplotlib\artist.py", line 62, in draw_wrapper
    draw(artist, renderer, *args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\patches.py", line 536, in draw
    renderer.draw_path(gc, tpath, affine, rgbFace)
  File "c:\projects\matplotlib\lib\matplotlib\patheffects.py", line 110, in draw_path
    rgbFace)
  File "c:\projects\matplotlib\lib\matplotlib\patheffects.py", line 211, in draw_path
    Stroke.draw_path(self, renderer, gc, tpath, affine, rgbFace)
  File "c:\projects\matplotlib\lib\matplotlib\patheffects.py", line 200, in draw_path
    renderer.draw_path(gc0, tpath, trans, rgbFace)
  File "c:\projects\matplotlib\lib\matplotlib\backends\backend_agg.py", line 166, in draw_path
    self._renderer.draw_path(gc, path, transform, rgbFace)
OverflowError: In draw_path: Exceeded cell block limit

Owner

tacaswell commented Dec 23, 2015

ex

FAIL: matplotlib.tests.test_path.test_xkcd.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python34_64\envs\test-environment\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 54, in failer
    result = f(*args, **kwargs)
  File "c:\projects\matplotlib\lib\matplotlib\testing\decorators.py", line 245, in do_test
    '(RMS %(rms).3f)'%err)
matplotlib.testing.exceptions.ImageComparisonFailure: images not close: C:\projects\matplotlib\result_images\test_path\xkcd.png vs. C:\projects\matplotlib\result_images\test_path\xkcd-expected.png (RMS 9.607)

from https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.338/job/nl9a32g98n9o9rov

@tacaswell tacaswell and 2 others commented on an outdated diff Dec 26, 2015

src/path_converters.h
+ static const uint32_t c = 2531011;
+ uint32_t m_seed;
+
+public:
+ RandomNumberGenerator() : m_seed(0) {}
+ RandomNumberGenerator(int seed) : m_seed(seed) {}
+
+ void seed(int seed)
+ {
+ m_seed = seed;
+ }
+
+ double get_double()
+ {
+ m_seed = (a * m_seed + c);
+ return (double)m_seed / (double)(1L << 32);
@tacaswell

tacaswell Dec 26, 2015

Owner

Is double 32 or 64bits? I wonder if this is what is causing the test failure on windows (in that this is no longer scaled [-1, 1] on all platforms?

@efiring

efiring Dec 26, 2015

Owner

double is 64 bits, so it looks to me like this should yield numbers from 0 to (2^32-1)/2^32. I can imagine the result of the division could differ depending on whether it is done using plain double-precision, or using extra-width floating point registers. Use of denormalized numbers is another option. There are compiler switches that can affect floating point results, so the same code on the same hardware can yield different results depending on the compiler and its options. Maybe this is what is happening here?

@efiring

efiring Dec 28, 2015

Owner

I think the 1L needs to be 1LL to ensure it is 64 bits.

@mdboom

mdboom Dec 29, 2015

Owner

It should be scaled [0, 1) on all platforms, since the seed is an unsigned int. I think @efiring is right here, that it should be 1LL. Let's try that and run it over the Windows tests.

@mdboom

mdboom Dec 29, 2015

Owner

That would certainly be consistent with a Linux vs. Windows problem. On a 64-bit machine, 1L is 64-bits on Linux and 32-bits on Windows. 1LL is 64 bits on both.

@mdboom

mdboom Dec 29, 2015

Owner

Sorry to spam -- reading further, however, the test failures on Windows are prior to this change, right? That could easily be explained by using the C stdlib RNG which varies entirely from platform to platform. This should (and seemingly does) address that problem by using exactly one RNG implementation everywhere.

@mdboom mdboom Use 1LL to avoid overflow
adafb96

@tacaswell tacaswell added a commit that referenced this pull request Dec 29, 2015

@tacaswell tacaswell Merge pull request #5728 from mdboom/local-random
Use custom RNG for sketch path
1446c69

@tacaswell tacaswell merged commit 1446c69 into matplotlib:master Dec 29, 2015

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 68.476%
Details

tacaswell removed the needs_review label Dec 29, 2015

@tacaswell tacaswell added a commit that referenced this pull request Dec 29, 2015

@tacaswell tacaswell Merge pull request #5728 from mdboom/local-random
Use custom RNG for sketch path
Conflicts:
	lib/matplotlib/tests/baseline_images/test_path/xkcd.pdf
	lib/matplotlib/tests/baseline_images/test_path/xkcd.png
	lib/matplotlib/tests/baseline_images/test_path/xkcd.svg

Resolved all by copying binaries from master branch
3be4490
Owner

tacaswell commented Dec 29, 2015

backported as 3be4490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment