Replaced std::random_shuffle with std::shuffle in tri by tybeller · Pull Request #24062 · matplotlib/matplotlib · GitHub

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.

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.

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.

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.

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?

@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

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

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

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.

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

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

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

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

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

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

Comment thread doc/api/next_api_changes/behavior/24062-tb.rst Outdated

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

Comment thread src/tri/_tri.h

oscargus reviewed Oct 23, 2022

View reviewed changes

Comment thread src/tri/_tri.h

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

@@ @@ -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.

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

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

                
                    No newline at end of file

Replaced std::random_shuffle with std::shuffle in tri by tybeller · Pull Request #24062 · matplotlib/matplotlib · GitHub

Repository navigation

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

Replaced std::random_shuffle with std::shuffle in tri#24062tacaswell merged 14 commits intomatplotlib:mainmatplotlib/matplotlib:mainfrom tybeller:shuffleFixtybeller/matplotlib:shuffleFixCopy head branch name to clipboard

Conversation

tybeller commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

tacaswell commented Oct 1, 2022

Uh oh!

ianthomas23 commented Oct 1, 2022

Uh oh!

tybeller commented Oct 1, 2022

Uh oh!

tybeller commented Oct 1, 2022

Uh oh!

rcomer commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianthomas23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oscargus Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

QuLogic Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

QuLogic Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

QuLogic Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

ianthomas23 commented Oct 12, 2022

Uh oh!

ianthomas23 commented Oct 12, 2022

Uh oh!

ianthomas23 commented Oct 13, 2022

Uh oh!

oscargus Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tacaswell commented Oct 28, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Replaced std::random_shuffle with std::shuffle in tri#24062
tacaswell merged 14 commits into
matplotlib:mainfrom
tybeller:shuffleFix

tybeller commented Oct 1, 2022 •
edited
Loading

rcomer commented Oct 1, 2022 •
edited
Loading