Conversation
ianna
left a comment
There was a problem hiding this comment.
@pfackeldey - Looks great! Please check the review comments. To summarize:
- strides property: use a proper exception (RuntimeError/NotImplementedError) and clarify why it’s unsupported.
- materialize method: fix grammar and make the error message more user-friendly, explaining what happened and how to report it.
- maybe_materialize helper: update docstring, clarify behavior for non-MaterializableArray args, and optionally make it more type-safe.
- VirtualNDArray conversion: improve readability, separate reason from solution, and consider user- vs. developer-friendly versions.
Overall: focus on clear, helpful error messages and consistent exception types, and ensure that all other documentation is written for end users. Thanks!
Co-authored-by: Ianna Osborne <ianna.osborne@cern.ch>
Co-authored-by: Ianna Osborne <ianna.osborne@cern.ch>
|
Hi @pfackeldey, the recent merges required a couple more VirtualArray -> VirtualNDArray renames + renames in the tests. Took care of that and ci should be passing again now. |
| warnings.warn( | ||
| "The `VirtualArray` class is deprecated and will be removed in a future release of Awkward Array. " | ||
| "Please plan to migrate your code to use the `VirtualNDArray` class instead.", |
There was a problem hiding this comment.
I would not say deprecated here. This message points to a potential difference between the classes and a migration effort required. This should make it clear that it's just a rename and no effort is required apart from a find-replace.
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3620 |
This PR does some refactoring for VirtualArrays:
VirtualArray->VirtualNDArray(closes Proposal: RenameVirtualArrayin_nplikes.virtualto avoid confusion withak.Array#3604)MaterializeableArrayPlaceholderArrayimplements its ownmaterializefunction that raises with a much better error than before; also we get rid of many assertions extra forPlaceholderArraysmaterialize_if_virtual->maybe_materialize(because now anyMaterializableArraycan materialize, not justVirtualNDArraysstridesto theArrayLikeprotocol, this solves also some previous typing issues that we ignored withtype: ignoreThis PR does not touch the
TypetracerArrayon purpose. If someone wants to adddef materialize(self)with touching-behavior to them at some point it could be possible, but I'd prefer to not touch this code path as it is carefully optimized.