Replaced std::random_shuffle with std::shuffle in tri #24062

tybeller · 2022-10-01T08:13:08Z

PR Summary

Replaced std::random_shuffle with std::shuffle for compliance with C++17. Deleted RandomNumberGenerator class. Included random library and used 32 bit mersenne twister as uniform random bit generator for shuffle. Note: TrapezoidMapTriFinder, triangulation interpolation, and refinement may yield different results due to this change. Closes #24010

PR Checklist

Tests and Styling

[N/A ] Has pytest style unit tests (and pytest passes).
[N/A ] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

[N/A] New features are documented, with examples if plot related.
[N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
[N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

…om_shuffle with shuffle and used mersenne twister engine to generate uniform random bit generator for the shuffle.

tacaswell · 2022-10-01T18:39:49Z

It looks like std::shuffle came in in c++11 which we just switch to having as our floor. It looks like moving that forwards was a good choice as it made this simple (I assume with enough #ifdef we could support a wider range of c++ versions....).

This seems reasonable and at least our tests are not sensitive to the different random order.

ianthomas23 · 2022-10-01T18:40:54Z

Thanks @tybeller. It is usual to mention the issue that the PR closes within the text (not just within the title) so that there is a link between the two and the issue automatically closes when the PR is merged.

Also, I said in the issue that it would need a release note but I realise now that this is classified as a behaviour change ("API that stays valid but will yield a different result") which therefore needs an API change note. The README file in the next_api_changes directory describes what is required here, see https://github.com/matplotlib/matplotlib/blob/main/doc/api/next_api_changes/README.rst and let me know if this is not clear enough and I will help out.

tybeller · 2022-10-01T20:19:23Z

Thank you, sorry for any issues as this is my first time. If I'm reading this right I need to add the issue within the pull request and add and API change note correct? I'll get working on that and please let me know if there is anything else I'm missing.

tybeller · 2022-10-01T20:21:06Z

One question, is there a way to edit these changes into this pull request or should I make a separate pull request with these fixes?

rcomer · 2022-10-01T21:31:33Z

@tybeller if you edit your original post here and put “closes #24010“, that will link the issue to the PR, see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Once you make any changes to you branch and push them to github, they will show in this pull request. There is advice for updating your branch in response to the review here: https://matplotlib.org/devdocs/devel/coding_guide.html#summary-for-pull-request-authors

Added API notes for changed output from TrapezoidMapTriFinder, triangulation interpolation, and refinement

ianthomas23

The code changes are fine, just a slight tweak to the API change note and this will be good to merge.

doc/api/next_api_changes/behavior/24062-tb.rst

Fix api change Co-authored-by: Ian Thomas <ianthomas23@gmail.com>

oscargus · 2022-10-04T18:27:05Z

doc/api/next_api_changes/behavior/24062-tb.rst

@@ -0,0 +1,3 @@
+Change in TrapezoidMapTriFinder, triangulation interpolation, and refinement output due to change in shuffle.
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Suggested change

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

would solve the failing doc build (if I got the length right...).

If you're having trouble aligning the underline, then that's a sign the title is probably too long. I would leave out all of the details and just say "Change in triangulation output ..." Also, no trailing period in titles.

QuLogic · 2022-10-04T23:39:31Z

doc/api/next_api_changes/behavior/24062-tb.rst

@@ -0,0 +1,3 @@
+Change in TrapezoidMapTriFinder, triangulation interpolation, and refinement output due to change in shuffle.
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


If you're having trouble aligning the underline, then that's a sign the title is probably too long. I would leave out all of the details and just say "Change in triangulation output ..." Also, no trailing period in titles.

QuLogic · 2022-10-04T23:40:20Z

doc/api/next_api_changes/behavior/24062-tb.rst

@@ -0,0 +1,3 @@
+Change in TrapezoidMapTriFinder, triangulation interpolation, and refinement output due to change in shuffle.
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+With std::random_shuffle removed from C++17, TrapezoidMapTriFinder, triangular grid interpolation and refinement now use std::shuffle with a new random generator which may give different results.


Different results how? Does a plot change? Is the order different? This is too vague and as a user I have no idea whether I need to do anything here. Also, please word-wrap lines at 80 characters.

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>

clarified that the conditions of the change in output

ianthomas23 · 2022-10-12T19:42:08Z

@tybeller The definition of class RandomNumberGenerator at the end of src/tri/_tri.h can be removed.

ianthomas23 · 2022-10-12T20:54:09Z

Here is a possible improved api change note:

``TrapezoidMapTriFinder`` uses different random number generator
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The random number generator used to determine the order of insertion of
triangle edges in ``TrapezoidMapTriFinder`` has changed. This can result in a
different triangle index being returned for a point that lies exactly on an
edge between two triangles. This can also affect triangulation interpolation
and refinement algorithms that use ``TrapezoidMapTriFinder``.

Updated with ianthomas23's API documentations

ianthomas23 · 2022-10-13T11:54:23Z

@tybeller Don't forget the changes in src/tri/_tri.h.

oscargus · 2022-10-14T07:06:34Z

src/tri/_tri.cpp

-    _seed = (_seed*_a + _c) % _m;
-    return (_seed*max_value) / _m;
-}
+}


Please end the file with an empty line. That will fix the pre-commit issue. (The pyside6 issue is fixed in #24158.)

added empty line at end

src/tri/_tri.h

tacaswell · 2022-10-28T14:07:06Z

Thank you for this work @tybeller and congratulations on your first merged Matplotlib PR 🎉 Hopefully we will hear from you again!

removed RandomNumberGenerator class, included <random>, replaced rand…

601d92a

…om_shuffle with shuffle and used mersenne twister engine to generate uniform random bit generator for the shuffle.

oscargus added the topic: tri label Oct 1, 2022

tacaswell added this to the v3.7.0 milestone Oct 1, 2022

tacaswell requested a review from ianthomas23 October 1, 2022 18:32

Merge branch 'matplotlib:main' into shuffleFix

4bf6696

tybeller changed the title ~~c++17 removed random_shuffle #24010~~ Replaced std::random_shuffle with std::shuffle in tri Oct 2, 2022

Create 24062-tb.rst

1ae3610

Added API notes for changed output from TrapezoidMapTriFinder, triangulation interpolation, and refinement

ianthomas23 requested changes Oct 4, 2022

View reviewed changes

doc/api/next_api_changes/behavior/24062-tb.rst Outdated Show resolved Hide resolved

Update doc/api/next_api_changes/behavior/24062-tb.rst

c8e0531

Fix api change Co-authored-by: Ian Thomas <ianthomas23@gmail.com>

oscargus reviewed Oct 4, 2022

View reviewed changes

QuLogic reviewed Oct 4, 2022

View reviewed changes

tybeller and others added 3 commits October 5, 2022 03:34

Update doc/api/next_api_changes/behavior/24062-tb.rst

bd3dc8a

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>

Update 24062-tb.rst

4648dda

Update 24062-tb.rst

d77b191

clarified that the conditions of the change in output

tybeller requested a review from ianthomas23 October 10, 2022 18:36

Merge branch 'matplotlib:main' into shuffleFix

0427513

tybeller added 2 commits October 12, 2022 19:08

Merge branch 'matplotlib:main' into shuffleFix

c4fb35e

Update 24062-tb.rst

e096d5e

Updated with ianthomas23's API documentations

tybeller added 2 commits October 14, 2022 01:46

Merge branch 'matplotlib:main' into shuffleFix

cf0613b

removed random number generator from _tri.h

d2d0d11

oscargus reviewed Oct 14, 2022

View reviewed changes

Update _tri.cpp

a06bae9

added empty line at end

ianthomas23 reviewed Oct 16, 2022

View reviewed changes

src/tri/_tri.h Show resolved Hide resolved

oscargus reviewed Oct 23, 2022

View reviewed changes

src/tri/_tri.h Show resolved Hide resolved

Update src/tri/_tri.h

3513a85

ianthomas23 approved these changes Oct 24, 2022

View reviewed changes

tacaswell merged commit 1bb5816 into matplotlib:main Oct 28, 2022

hoodmane mentioned this pull request Oct 28, 2022

c++17 removed random_shuffle #24010

Closed

CGDogan mentioned this pull request Aug 9, 2023

Bump matplotlib to +3.6.0 to make it work on ARM devices choosehappy/HistoQC#260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced std::random_shuffle with std::shuffle in tri #24062

Replaced std::random_shuffle with std::shuffle in tri #24062

tybeller commented Oct 1, 2022 •

edited

Loading

tacaswell commented Oct 1, 2022

ianthomas23 commented Oct 1, 2022

tybeller commented Oct 1, 2022

tybeller commented Oct 1, 2022

rcomer commented Oct 1, 2022 •

edited

Loading

ianthomas23 left a comment

oscargus Oct 4, 2022

QuLogic Oct 4, 2022

QuLogic Oct 4, 2022

QuLogic Oct 4, 2022

ianthomas23 commented Oct 12, 2022

ianthomas23 commented Oct 12, 2022

ianthomas23 commented Oct 13, 2022

oscargus Oct 14, 2022

tacaswell commented Oct 28, 2022

		@@ -0,0 +1,3 @@
		Change in TrapezoidMapTriFinder, triangulation interpolation, and refinement output due to change in shuffle.
		~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Replaced std::random_shuffle with std::shuffle in tri #24062

Replaced std::random_shuffle with std::shuffle in tri #24062

Conversation

tybeller commented Oct 1, 2022 • edited Loading

PR Summary

PR Checklist

tacaswell commented Oct 1, 2022

ianthomas23 commented Oct 1, 2022

tybeller commented Oct 1, 2022

tybeller commented Oct 1, 2022

rcomer commented Oct 1, 2022 • edited Loading

ianthomas23 left a comment

Choose a reason for hiding this comment

oscargus Oct 4, 2022

Choose a reason for hiding this comment

QuLogic Oct 4, 2022

Choose a reason for hiding this comment

QuLogic Oct 4, 2022

Choose a reason for hiding this comment

QuLogic Oct 4, 2022

Choose a reason for hiding this comment

ianthomas23 commented Oct 12, 2022

ianthomas23 commented Oct 12, 2022

ianthomas23 commented Oct 13, 2022

oscargus Oct 14, 2022

Choose a reason for hiding this comment

tacaswell commented Oct 28, 2022

tybeller commented Oct 1, 2022 •

edited

Loading

rcomer commented Oct 1, 2022 •

edited

Loading