Skip to content

fix: properly handle VirtualArray in nplike.asarray#3597

Merged
ianna merged 18 commits into
scikit-hep:mainfrom
ikrommyd:properly-handle-virtual-in-asarray
Aug 27, 2025
Merged

fix: properly handle VirtualArray in nplike.asarray#3597
ianna merged 18 commits into
scikit-hep:mainfrom
ikrommyd:properly-handle-virtual-in-asarray

Conversation

@ikrommyd

@ikrommyd ikrommyd commented Aug 3, 2025

Copy link
Copy Markdown
Member

I realized that nplike.asarray does not properly handle VirtualArray. A copy should always be created when copy=True and that's not the case. The virtual array code branch in asarray never cared about copying the underying buffer at all. The codebase often does nplike.asarray(some object, copy=True) exactly because it needs a copy to do in-place modifications. This would cause problems if some object is a virtual array.

Also very lightly refactoring reshape without changing its functionality just to match the general style of other functions that create new virtual arrays.

@codecov

codecov Bot commented Aug 3, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.54%. Comparing base (b749e49) to head (c55ccdc).
⚠️ Report is 408 commits behind head on main.

Additional details and impacted files
Files with missing lines Coverage Δ
src/awkward/_nplikes/array_module.py 95.20% <100.00%> (+8.82%) ⬆️

... and 195 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/awkward/_nplikes/array_module.py

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

@ikrommyd - thanks for looking into it. This should be a part of the whole VirtualArray code refactoring.
I'm adding 'request changes' because we don't want any patches while refactoring. I think, it would be better to move it to a list of issues. Thanks for understanding!

@ikrommyd

ikrommyd commented Aug 3, 2025

Copy link
Copy Markdown
Member Author

This is not about refactoring. Not copying when the code should copy is just wrong and can potentially lead to silently wrong results when the code wants to do in place modifications to arrays that should be a copy but are not. This is a somewhat “critical” fix potentially although I don’t know how often such an error might happen. Please take it over and do as you please with it.

@ikrommyd

ikrommyd commented Aug 3, 2025

Copy link
Copy Markdown
Member Author

This is very dangerously wrong and this PR fixes it:

In [1]: import awkward as ak
   ...: import numpy as np
   ...:
   ...: from awkward._nplikes.virtual import VirtualArray, materialize_if_virtual
   ...: from awkward._nplikes.numpy import Numpy
   ...: from awkward._nplikes.shape import unknown_length
   ...:
   ...: numpy = Numpy.instance()
   ...:
   ...: def shape_generator():
   ...:     print("CALLING SHAPE GENERATOR!")
   ...:     return (10,)
   ...:
   ...: def data_generator():
   ...:     print("CALLING DATA GENERATOR!")
   ...:     return np.array([0, 1, 2, 3, 4, 5, 6, 7, 8, 9], dtype=np.int32)
   ...:
   ...: v = VirtualArray(numpy, (unknown_length,), np.int32, data_generator, shape_generator)

In [2]: v2 = numpy.asarray(v, copy=True)

In [3]: v2[0] = 111
CALLING DATA GENERATOR!

In [4]: v2
Out[4]: VirtualArray(array=[111   1   2   3   4   5   6   7   8   9], dtype('int32'), shape=(10,))

In [5]: v
Out[5]: VirtualArray(array=[111   1   2   3   4   5   6   7   8   9], dtype('int32'), shape=(10,))

Comment thread src/awkward/_nplikes/array_module.py
Comment thread src/awkward/_nplikes/array_module.py
@ianna ianna added the pr-next-release Required for the next release label Aug 14, 2025
Comment thread src/awkward/_nplikes/array_module.py
@ianna
ianna self-requested a review August 26, 2025 16:13
@ianna

ianna commented Aug 26, 2025

Copy link
Copy Markdown
Member

@ikrommyd - please, check:

src/awkward/_nplikes/array_module.py:368: error: Argument 1 to "reshape" of "ArrayModuleNumpyLike" has incompatible type "ArrayLike"; expected "ArrayLikeT | PlaceholderArray"  [arg-type]
src/awkward/_nplikes/array_module.py:368: note: Error code "arg-type" not covered by "type: ignore" comment

@ikrommyd

ikrommyd commented Aug 26, 2025

Copy link
Copy Markdown
Member Author

@ikrommyd - please, check:

src/awkward/_nplikes/array_module.py:368: error: Argument 1 to "reshape" of "ArrayModuleNumpyLike" has incompatible type "ArrayLike"; expected "ArrayLikeT | PlaceholderArray"  [arg-type]
src/awkward/_nplikes/array_module.py:368: note: Error code "arg-type" not covered by "type: ignore" comment

Yeah I'm just ignoring it for now (like we do everywhere else). The proper fix would be to resolve #3429 and properly type the nplikes better.

@ikrommyd

Copy link
Copy Markdown
Member Author

I think this is fine now. @pfackeldey could you also take a look please for an unbiased view?

@ikrommyd
ikrommyd requested a review from pfackeldey August 26, 2025 16:39
@github-actions

Copy link
Copy Markdown

The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3597

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

@ikrommyd - thanks, looks great.

@ianna
ianna merged commit cc543bf into scikit-hep:main Aug 27, 2025
46 checks passed
@ikrommyd
ikrommyd deleted the properly-handle-virtual-in-asarray branch August 27, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-next-release Required for the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants