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 (andpytest passes).
[N/A ] IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

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 thev3.7.0 milestone

Oct 1, 2022

tacaswell requested a review fromianthomas23

October 1, 2022 18:32

Copy link

Member

tacaswell commentedOct 1, 2022

It looks likestd::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.

Copy link

Member

ianthomas23 commentedOct 1, 2022

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. TheREADME file in thenext_api_changes directory describes what is required here, seehttps://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.

Copy link

ContributorAuthor

tybeller commentedOct 1, 2022

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.

Copy link

ContributorAuthor

tybeller commentedOct 1, 2022

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?

Copy link

Member

rcomer commentedOct 1, 2022•
edited
Loading

@tybeller if you edit your original post here and put “closes#24010“, that will link the issue to the PR, seehttps://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

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

Copy link

Member

ianthomas23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 OutdatedShow resolvedHide resolved

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

c8e0531

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

oscargus reviewed

Oct 4, 2022

View reviewed changes

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

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

Copy link

Member

oscargusOct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change

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

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

Copy link

Member

QuLogicOct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 reviewed

Oct 4, 2022

View reviewed changes

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

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

Copy link

Member

QuLogicOct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Copy link

Member

QuLogicOct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

tybellerand others added3 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 fromianthomas23

October 10, 2022 18:36

Merge branch 'matplotlib:main' into shuffleFix

0427513

Copy link

Member

ianthomas23 commentedOct 12, 2022

@tybeller The definition ofclass RandomNumberGenerator at the end ofsrc/tri/_tri.h can be removed.

Copy link

Member

ianthomas23 commentedOct 12, 2022

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 oftriangle edges in ``TrapezoidMapTriFinder`` has changed. This can result in adifferent triangle index being returned for a point that lies exactly on anedge between two triangles. This can also affect triangulation interpolationand refinement algorithms that use ``TrapezoidMapTriFinder``.

tybeller added2 commits

October 12, 2022 19:08

Merge branch 'matplotlib:main' into shuffleFix

c4fb35e

Update 24062-tb.rst

e096d5e

Updated with ianthomas23's API documentations

Copy link

Member

ianthomas23 commentedOct 13, 2022

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

tybeller added2 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

src/tri/_tri.cpp Outdated

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

Copy link

Member

oscargusOct 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Update _tri.cpp

a06bae9

added empty line at end

ianthomas23 reviewed

Oct 16, 2022

View reviewed changes

src/tri/_tri.hShow resolvedHide resolved

oscargus reviewed

Oct 23, 2022

View reviewed changes

src/tri/_tri.hShow resolvedHide resolved

Update src/tri/_tri.h

3513a85

ianthomas23 approved these changes

Oct 24, 2022

View reviewed changes

tacaswell merged commit1bb5816 intomatplotlib:main

Oct 28, 2022

Copy link

Member

tacaswell commentedOct 28, 2022

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

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 deviceschoosehappy/HistoQC#260

Merged

Labels

topic: tri

6 participants

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

tybeller commentedOct 1, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

tacaswell commentedOct 1, 2022

Uh oh!

ianthomas23 commentedOct 1, 2022

Uh oh!

tybeller commentedOct 1, 2022

Uh oh!

tybeller commentedOct 1, 2022

Uh oh!

rcomer commentedOct 1, 2022• editedLoading 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!

oscargusOct 4, 2022

Choose a reason for hiding this comment

Uh oh!

QuLogicOct 4, 2022

Choose a reason for hiding this comment

Uh oh!

QuLogicOct 4, 2022

Choose a reason for hiding this comment

Uh oh!

QuLogicOct 4, 2022

Choose a reason for hiding this comment

Uh oh!

ianthomas23 commentedOct 12, 2022

Uh oh!

ianthomas23 commentedOct 12, 2022

Uh oh!

ianthomas23 commentedOct 13, 2022

Uh oh!

oscargusOct 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tacaswell commentedOct 28, 2022

Uh oh!

Uh oh!

tybeller commentedOct 1, 2022•
edited
Loading

rcomer commentedOct 1, 2022•
edited
Loading