fix: properly handle VirtualArray in nplike.asarray#3597
fix: properly handle VirtualArray in nplike.asarray#3597ianna merged 18 commits intoscikit-hep:mainfrom
VirtualArray in nplike.asarray#3597Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
ianna
left a comment
There was a problem hiding this comment.
@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!
|
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. |
|
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,)) |
|
@ikrommyd - please, check: |
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 |
|
I think this is fine now. @pfackeldey could you also take a look please for an unbiased view? |
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3597 |
I realized that
nplike.asarraydoes not properly handleVirtualArray. A copy should always be created whencopy=Trueand that's not the case. The virtual array code branch inasarraynever cared about copying the underying buffer at all. The codebase often doesnplike.asarray(some object, copy=True)exactly because it needs a copy to do in-place modifications. This would cause problems ifsome objectis a virtual array.Also very lightly refactoring
reshapewithout changing its functionality just to match the general style of other functions that create new virtual arrays.