Skip to content

Support for NumPy BitGenerators PR#5: Generator Shuffling Methods. - #8042

Merged
sklam merged 12 commits into
numba:mainfrom
kc611:np_gen_shuffling_methods
Sep 21, 2022
Merged

sklam merged 12 commits into
numba:mainfrom
kc611:np_gen_shuffling_methods

Conversation

@kc611

@kc611 kc611 commented May 12, 2022 •

Copy link
Copy Markdown
Contributor

This PR builds on top of #8041

Currently has the following Generator methods:

  • Generator().shuffle()
  • Generator().permutation()

@kc611 kc611 changed the title Np gen shuffling methods Support for Numpy BitGenerators PR#5: Geneator Shuffling Methods. May 12, 2022
@sklam sklam added this to the Numba 0.57 RC milestone Jun 1, 2022
@esc
esc self-requested a review July 12, 2022 14:49
@kc611

kc611 commented Jul 14, 2022

Copy link
Copy Markdown
Contributor Author

Reminder: Make sure the masks functionality is being used correctly, if it's not, remove it within this PR: #8041 (comment)

@kc611
kc611 force-pushed the np_gen_shuffling_methods branch from 929fb49 to cf9a039 Compare August 8, 2022 11:18
@kc611
kc611 marked this pull request as ready for review August 9, 2022 13:04
@esc

esc commented Aug 29, 2022

Copy link
Copy Markdown
Member

@kc611 thank you for opening this PR, your efforts to improve Numba are appreciated. It's good to see the axis support on this. I just looked at the error messages coming from NumPy and compared them to the ones coming from Numba. I think it should be possible to match the errors:

In [21]: import numpy as np

In [22]: from numba import njit

In [23]: @njit
    ...: def foo(rng, arr, axis):
    ...:     rng.shuffle(arr, axis=axis)
    ...:

In [24]: a = np.arange(9).reshape(3,3)

In [25]: rng.shuffle(a,axis=2)
---------------------------------------------------------------------------
AxisError                                 Traceback (most recent call last)
Input In [25], in <cell line: 1>()
----> 1 rng.shuffle(a,axis=2)

File _generator.pyx:4438, in numpy.random._generator.Generator.shuffle()

AxisError: axis 2 is out of bounds for array of dimension 2

In [26]: foo(rng, a, axis=1)

In [27]: foo(rng, a, axis=2)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
Input In [27], in <cell line: 1>()
----> 1 foo(rng, a, axis=2)

IndexError: tuple index out of range

@esc esc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think, since Numbers are only being "moved around" there should not be any ulp deviation between the expected result and the received one.

Comment thread numba/tests/test_np_randomgen.py Outdated
Comment thread numba/tests/test_np_randomgen.py Outdated
@esc esc added Effort - medium Medium size effort needed 4 - Waiting on author Waiting for author to respond to review labels Aug 29, 2022
@esc

esc commented Aug 29, 2022 •

Copy link
Copy Markdown
Member

Using the review cheklist from https://github.com/numba/numba/pull/8209/files I have checked that:

  • There is no other Pull Request proposing the same change. (If there is
    please highlight this and one of the core developers will pick it up.)
  • The proposed additional NumPy functionality is not deprecated by
    NumPy.
  • Public CI is passing.
  • The patch does not touch files in numba.core.
  • The patch is implemented using the high-level extension API only
    (https://numba.readthedocs.io/en/stable/extending/high-level.html).
  • The patch updates the documentation to reflect the new functionality.
    (https://github.com/numba/numba/blob/main/docs/source/reference/numpysupported.rst)
  • The patch is line wrapped to 80 characters.
  • Changes to Python source in the patch are flake8 compliant.
  • The implementation of the NumPy function reflects that present in the
    NumPy implementation itself (look in the NumPy repository:
    https://github.com/numpy/numpy)...
    • The implementation has references to the region of the NumPy code
      base that provides the same functionality. The references are URLs
      of the form:
      https://github.com/numpy/numpy/tree/<SHA>/path/to/file.py:#<lines>
    • The code "looks" reasonably similar to the NumPy implementation.
    • The code uses the same algorithm as the NumPy implementation, if
      it does not it explains why and this justification is reasonable.
    • The type inference part of the Numba implementation restricts the
      types to those declared in the NumPy docs (or if not present, what
      is accepted in the NumPy implementation itself).
    • The Numba implementation uses the same variable names for
      arguments as in the NumPy implementation.
    • The Numba code has commentary present for anything not-obvious by
      inspection.
    • The Numba code accommodates differences that might occur across
      different NumPy versions in a clear fashion and uses
      numba.np.numpy_support.numpy_version to separate the difference
      in implementations.
  • The unit tests for the implementation:
    • Use the same test inputs as the NumPy unit tests for the same
      functionality. If this is not the case there are comments which
      explain why and this justification is reasonable.
    • The NumPy unit test inputs have reference URLs associated with
      them to track their origin. The references are URLs of the form:
      https://github.com/numpy/numpy/tree/<SHA>/path/to/file.py:#<lines>
    • Use unittest style assertions (not pytest style!).
    • Check that unsupported/invalid input types are handled as expected
      using the with self.assertRaises() pattern.
    • Explicitly check custom error messages are raised appropriately.
    • Hit all the branches of the proprosed change (both at type
      inference and run time).
    • Check that numerically awkward values work as expected e.g.
      NaN, +/-Inf, 0, INT_MAX, INT_MIN etc.
    • Check that types which can be converted to arrays (assuming NumPy
      accepts this behaviour) work as expected.
    • Check that passing empty versions of containers e.g. empty tuple
      or 0d array works as expected.
  • The patch works in a simple manual test in my local checkout.
  • I cannot break the code in the patch in my local checkout after
    trying to do so for a minimum of 10 minutes. The error messages seem
    reasonable and there are no "incorrect" results.
  • I think the code introduced in this change is easy to maintain.
  • I understand all the code in this PR.

esc
esc previously requested changes Aug 31, 2022

@esc esc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that public CI is currently broken, probably due to a typo. I think the following suggestion should remedy this.

Comment thread numba/np/random/generator_methods.py Outdated
@esc esc added 4 - Waiting on CI Review etc done, waiting for CI to finish 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review 4 - Waiting on author Waiting for author to respond to review 4 - Waiting on CI Review etc done, waiting for CI to finish labels Sep 2, 2022

@stuartarchibald stuartarchibald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch @kc611 and review @esc. I've take a look and have asked a few questions that might be good to resolve ahead of merge.

Comment thread numba/np/random/generator_methods.py Outdated
Comment thread numba/np/random/generator_methods.py Outdated
Comment thread numba/np/random/random_methods.py
Comment thread numba/tests/test_np_randomgen.py
Comment thread numba/np/random/generator_methods.py Outdated
@esc esc added 4 - Waiting on author Waiting for author to respond to review and removed 5 - Ready to merge Review and testing done, is ready to merge labels Sep 6, 2022
@esc

esc commented Sep 7, 2022

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Contributor
Azure Pipelines successfully started running 1 pipeline(s).

@stuartarchibald stuartarchibald changed the title Support for Numpy BitGenerators PR#5: Geneator Shuffling Methods. Support for NumPy BitGenerators PR#5: Generator Shuffling Methods. Sep 8, 2022
@esc

esc commented Sep 20, 2022

Copy link
Copy Markdown
Member

Testing at numba_smoketest_cpu_nprgen_30

@esc

esc commented Sep 21, 2022

Copy link
Copy Markdown
Member

Testing at numba_smoketest_cpu_nprgen_30

This was all green.

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed and removed 4 - Waiting on author Waiting for author to respond to review labels Sep 21, 2022
@sklam
sklam requested a review from esc September 21, 2022 22:18
@sklam
sklam dismissed esc’s stale review September 21, 2022 22:19

reviewer approved

@sklam
sklam merged commit 5d5943a into numba:main Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed Effort - medium Medium size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants