Update vector_of_array.jl - Githubissues

SciML / RecursiveArrayTools.jl

Tools for easily handling objects like arrays of arrays and deeper nestings in scientific machine learning (SciML) and other applications

https://docs.sciml.ai/RecursiveArrayTools/stable/

Other

212 stars 57 forks source link

Update vector_of_array.jl #355

Closed lxvm closed 7 months ago

lxvm commented 7 months ago

Fix #354

Checklist

[x] Appropriate tests were added
[ ] Any code changes were done in a way that does not break public API
[ ] All documentation related to code changes were updated
[ ] The new code follows the contributor guidelines, in particular the SciML Style Guide and COLPRAC.
[ ] Any new documentation only uses public API

Additional context

Add any other context about the problem here.

lxvm commented 7 months ago

Oops this might violate the copying semantics of getindex

lxvm commented 7 months ago

I meant to say that getindex must copy, but Array may not. Is Adapt.adapt required to copy?

AayushSabharwal commented 7 months ago

I think it copies, and anyway Array(::VectorOfArray) will copy anyway?

lxvm commented 7 months ago

The following method of Array doesn't copy: https://github.com/SciML/RecursiveArrayTools.jl/blob/351cb407e82a516be1269fa5408e45cc3b672a31/src/vector_of_array.jl#L89 I'll probably keep the original method and add one with numeric elements that forwards the indices to the inner array.

lxvm commented 7 months ago

I updated my pr to just add a new method which will fix the broken case. I also rebased onto the main branch. I hope this looks good.