Sketch seed #26050
Sketch seed #26050
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
Thank you for your contribution @AryanSheka! Please put some information in the PR summary to tell us about the problem you are solving, with links to the issue being addressed and to the PR this replaces. You will also need to rebase your branch to fix a conflict in a file that has changed both here and on our If you need help, feel free to ask questions here. Or you may prefer to ask them in our incubator gitter room. |
|
Hi, I'm sorry I'm very new to this so I'm very confused about everything. |
|
Awesome, thanks for pulling this together! No need to apologize, we'd be happy to help you out w/ whatever you're confused about if you tell us what's tripping you up. We also have a new contributors meeting next tuesday that may be of help https://scientific-python.org/calendars/ |
|
I have tried to fix the code as much as possible, but still some tests are failing and I do not know how to fix it. Can someone please |
|
So the two tests that are failing are the xkcd image comparison tests, which means that this PR is probably messing with the default sketch. You can compare the expected and actual figures visually, see the image comparison test docs for how to find them https://matplotlib.org/devdocs/devel/testing.html#writing-an-image-comparison-test |
|
So basically the image comparision test compares the expected image to the one we get from plotting. But if we see the main issue #13479 we are changing the sketch transformation of xkcd to make it give us a randomized pattern result rather than the repeating pattern result like the expected image. |
|
If I have understood the code comment, the pattern will be different than it was before, but it will still be deterministic because it just adds one to the previous seed value matplotlib/src/path_converters.h Lines 1001 to 1003 in 921ebda So I think it should be enough to replace the baseline image with the result image that you now get? |
|
Everytime I try to run the pytest in order to generate the images, I am always hit with the following error: ImportError: cannot import name '_c_internal_utils' from partially initialized module 'matplotlib' (most likely due to a circular import) (/workspaces/matplotlib/lib/matplotlib/init.py) I have tried fixing it but was unsuccessful. |
|
Did you rebase since you installed Matplotlib? If so I think you will need to install it again as the C code will have to be re-compiled. |
|
Well I have run into an issue, the expected image of xkcd and xkcd markers are: But we are getting :
|
|
@AryanSheka are you sure what you're showing isn't the diff (expected-actual) image? |
|
I'm sorry my bad, I thought they were the same thing, I have updated the baseline images and am still getting an image_comparision error. xkcd-expected xkcd-recieved xkcd-failed-diff It doesnt matter how many times I replace the baseline_images, the new one produced is always slightly (not very noticable) different from the old one and the test fails. Even if you reinstall matplotlib after replacing the images, and then test it , they fail. |
32087b7 to
0b03de1
Compare
|
@AryanSheka can you do me a favor and just try to generate the image twice not as part of the test suite and check if they're the same? |
Yep here are 2 images generated together with the exact same parameters.
An online image comparision shows a small difference in the marked area even though the parameters were same. |
|
@AryanSheka well done, I think now someone only needs to figure out where to allocate |
f200920 to
170fddf
Compare
|
I have tried searching for other ways to allocate value to Sketch::previous_seed from the past few days, I could not find any other way to do this other than the way it is done in the code ( ie create path_converters.cpp and allocate value to Sketch::previous_seed in it). At this point I do not know what to do further. |
I tried and also did not succeed, it needs to be allocated both in |
Well I have added tests, I will tell the significance of each of them: This tests whether passing a seed to the Artist.set_sketch actually changes the global rcParam This tests the same but for xkcd matplotlib/lib/matplotlib/tests/test_path.py Line 246 in d2dc510 This tests xkcd with default parameters (seed==0). This indirectly also tests whether passing a seed to xkcd impacts the sketch. matplotlib/lib/matplotlib/tests/test_path.py Line 252 in d2dc510 This tests whether setting a seed through artist impacts the sketch (it should) matplotlib/lib/matplotlib/tests/test_path.py Line 278 in d2dc510 This tests whether changing the rcParam impacts the sketch (it should). matplotlib/lib/matplotlib/tests/test_path.py Line 288 in d2dc510 Overall these tests cover all the changes made in this pr(including seed rolling) and can ensure that future commits do not break the changes the made in this pr. Note: The image comparision tests also test the rolling charecteristic of seed |
|
The relevant failures here are Corresponding diffs are: |
d81bcae to
6a1d20e
Compare
There was a problem hiding this comment.
So the reason I'm kind of advocating using https://matplotlib.org/devdocs/api/testing_api.html#matplotlib.testing.decorators.check_figures_equal after verifying that the seed isn't 0 is b/c there's not a great here to verify that the image is using the passed in seed.
| @@ -823,7 +823,8 @@ def draw(self, renderer): | |||
| gc.set_foreground(ec_rgba, isRGBA=True) | |||
| if self.get_sketch_params() is not None: | |||
| scale, length, randomness = self.get_sketch_params() | |||
| gc.set_sketch_params(scale/2, length/2, 2*randomness) | |||
| seed = self._sketch_seed | |||
| gc.set_sketch_params(scale/2, length/2, 2*randomness, seed) | |||
There was a problem hiding this comment.
since this isn't incremented, are you sure that the rolling is correctly implemented?
like xkcd(seed=1234)
ax1.plot([1,2,3])
ax2.plot([1,2,3])
should have lines that look different - and yes please test if you haven't
| m_rand(0), | ||
| m_seed(seed) |
There was a problem hiding this comment.
I'm confused what's going on here
672fd58 to
11954ea
Compare
|
Apologies for the delay in making the changes. Have been quite busy the past few week and also will be busy for some time in the future (possibly next few weeks), hence I wont be able to work on this pr during that time. In the mean time anyone can feel free to finish this. |
Seed can be manually set for path.sketch by modifying the value of rcParams path.sketch_seed or by passing a seed value to xkcd or Artist.set_sketch . Seed will also have a rolling(auto incrementing) behaviour. Co-Authored-By: Oscar Gustafsson <8114497+oscargus@users.noreply.github.com> Co-Authored-By: eudoxos <1029876+eudoxos@users.noreply.github.com>
|
Thanks for all the work so far, I rebased it to make it a little bit easier to review. |
|
Hey @AryanSheka, just wanted to check about your availability for finishing this? otherwise I'll write finish this out. |
|
Hey @story645, I don't think so I will be able to contribute to this pr further. Please feel free to move ahead. |
PR summary
The pr is related to #25796
closes #13479
rebased from @oscargus
PR checklist