Fix issue 8370 by bszollosinagy · Pull Request #8376 · numba/numba

bszollosinagy · 2022-08-21T19:30:14Z

New unit tests were created, and the solution added, as suggested by @gmarkall

All unit tests pass locally that would exercise ravel() and flatten()

python -m numba.runtests -m -v numba.tests.test_array_reductions.TestArrayReductions
python -m numba.runtests -m -v numba.tests.test_array_manipulation.TestArrayManipulation
python -m numba.runtests -m -v numba.tests.test_closure.TestObjmodeFallback
python -m numba.runtests -m -v numba.tests.test_dyn_array.TestNpStack
python -m numba.runtests -m -v numba.tests.test_np_functions.TestNPFunctions.test_extract_basic
python -m numba.runtests -m -v numba.tests.test_support.TestAssertPreciseEqual

CUDA tests may need to be run, but I simply cannot download the CUDA drivers from Nvidia today for some reason, so maybe a CI test would be more appropriate.

…umba#8370

gmarkall · 2022-08-22T09:45:21Z

Many thanks for the PR!

CUDA tests may need to be run, but I simply cannot download the CUDA drivers from Nvidia today for some reason, so maybe a CI test would be more appropriate.

I'm not sure flatten would work on CUDA since its implementation returns a copy in Numba, and it's not possible to do allocation for the CUDA target - am I missing something here?

gmarkall · 2022-08-22T09:45:55Z

gpuci run tests

gmarkall · 2022-08-22T09:47:47Z

(Marking as RFR because the test fail is a CI issue, not an issue with this PR)

… whether a copy was necessary or not.

…er a copy is needed

…nit test.

… copy.

… running a flatten()

bszollosinagy · 2022-09-04T20:39:10Z

Resolved issue picked up by an automated test: if the source array is mutable, then a simple ravel() should not make it read-only.

bszollosinagy · 2022-09-04T20:39:51Z

Ready for review.

bszollosinagy · 2022-09-04T20:54:54Z

Note: the algo modification is not a big deal:

on calling flatten(), a copy is made by Numba, and therefore the read-only is removed (just like in Numpy, see ref 1).
In the case of ravel(), the copy is only made if the "order" of the dimensions has to be converted from Fortran to C, otherwise no copy is made.
- In case there was a copy, then the read-only is removed (just like in Numpy, see ref 1),
- in case no copy was needed, then we look at the source array: was it read-only.

That's all.

ref: Numpy removing writeable=False when doing a copy()

gmarkall · 2023-01-10T14:40:15Z

/azp run

gmarkall · 2023-01-10T14:40:21Z

gpuci run tests

azure-pipelines · 2023-01-10T14:40:28Z

Azure Pipelines successfully started running 1 pipeline(s).

gmarkall

Many thanks for the updates and your continued efforts! There are a couple of comments on the PR.

In addition, in test_readonly_after_ravel and test_readonly_after_flatten, there is no assertion that checks that an expected output is produced - could you also assert that cfunc and pyfunc both produce the same return value please?

gmarkall · 2023-01-10T14:52:58Z

Also, apologies for the delay in this round of review.

…ause different version of Numba return different way to say that you are trying to modify a read-only array. * added an assertion to check the Numpy and Numba return the same results.

gmarkall

Many thanks for the quick update - just a very small comment on the diff - would you be happy to make this change, then this should be good to approve?

…rting

gmarkall · 2023-01-11T11:48:46Z

Many thanks - just waiting on CI before I approve.

gmarkall · 2023-01-11T12:00:34Z

gpuci run tests

gmarkall

Many thanks for the PR and updates! I think this looks good now.

bszollosinagy added 4 commits August 21, 2022 21:21

added unit test to reproduce issue numba#8370

72bbc5e

added a unit test that reproduces another suggested problem in Issue n…

6f366f5

…umba#8370

added fix of issue numba#8370, as suggested by gmarkall.

bfae727

added fix for ravel() in the same spirit as the fix for issue 8370

2d8ac7a

bszollosinagy requested review from sklam and stuartarchibald as code owners August 21, 2022 19:30

gmarkall added the 3 - Ready for Review label Aug 22, 2022

stuartarchibald reviewed Aug 22, 2022

View reviewed changes

Comment thread numba/core/typing/arraydecl.py Outdated

bszollosinagy added 3 commits August 22, 2022 13:48

added test to see if the writeable flag is set after ravel() based on…

c24796d

… whether a copy was necessary or not.

mutability of the array resulting from ravel() is determined by wheth…

f3f12c6

…er a copy is needed

Streamlined a boolean expression, and added documentation string to u…

5991a55

…nit test.

stuartarchibald added the Effort - medium Medium size effort needed label Aug 23, 2022

gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Aug 23, 2022

gmarkall self-assigned this Aug 23, 2022

gmarkall added this to the Numba 0.57 RC milestone Aug 23, 2022

bszollosinagy added 4 commits September 4, 2022 00:42

restart CI tests

2d70048

Merge branch 'main' into fix_issue_8370

b43a84a

only set read-only, when the source is read-only and we will not do a…

d4b40c0

… copy.

added clarification about why we always remove the readonly flag when…

ac22f68

… running a flatten()

gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Sep 6, 2022

gmarkall requested changes Jan 10, 2023

View reviewed changes

Comment thread numba/tests/test_array_manipulation.py Outdated

Comment thread numba/tests/test_array_manipulation.py Outdated

Comment thread numba/core/typing/arraydecl.py Outdated

Comment thread numba/core/typing/arraydecl.py Outdated

gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jan 10, 2023

bszollosinagy added 4 commits January 10, 2023 19:27

Merge branch 'numba:main' into fix_issue_8370

84df01f

rephrased the comment about how we ensure identical behaviour with NumPy

61fb113

typo: NumPy

8f64f1b

* removed the name of the exception expected when the test fails, bec…

b1fd752

…ause different version of Numba return different way to say that you are trying to modify a read-only array. * added an assertion to check the Numpy and Numba return the same results.

bszollosinagy requested review from gmarkall and removed request for sklam January 10, 2023 20:46

gmarkall reviewed Jan 11, 2023

View reviewed changes

Comment thread numba/tests/test_array_manipulation.py Outdated

gmarkall requested changes Jan 11, 2023

View reviewed changes

* now uses np.testing.assert_array_equal for more detailed error repo…

ea8f5e8

…rting

gmarkall added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on author Waiting for author to respond to review labels Jan 11, 2023

gmarkall added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jan 11, 2023

gmarkall approved these changes Jan 11, 2023

View reviewed changes

sklam merged commit 6c6fe2b into numba:main Jan 11, 2023

Conversation

bszollosinagy commented Aug 21, 2022

Uh oh!

gmarkall commented Aug 22, 2022

Uh oh!

gmarkall commented Aug 22, 2022

Uh oh!

gmarkall commented Aug 22, 2022

Uh oh!

Uh oh!

bszollosinagy commented Sep 4, 2022

Uh oh!

bszollosinagy commented Sep 4, 2022

Uh oh!

bszollosinagy commented Sep 4, 2022

Uh oh!

gmarkall commented Jan 10, 2023

Uh oh!

gmarkall commented Jan 10, 2023

Uh oh!

azure-pipelines Bot commented Jan 10, 2023

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmarkall commented Jan 10, 2023

Uh oh!

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

gmarkall commented Jan 11, 2023

Uh oh!

gmarkall commented Jan 11, 2023

Uh oh!

gmarkall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants