search

SciML / LabelledArrays.jl

Arrays which also have a label for each element for easy scientific machine learning (SciML)
https://docs.sciml.ai/LabelledArrays/stable/
Other
120 stars 21 forks source link

Add vec definition instead of relying on StaticArrays #101

Closed chriselrod closed 3 years ago

chriselrod commented 3 years ago

fixes #100 Maybe it'd be better to spend the time to figure out what went wrong in StaticArrays, but I find it tends to take several weeks to get anything merged into StaticArrays, and defining a one liner is easy.

mateuszbaran commented 3 years ago

I'd suggest defining reshape instead: @inline reshape(a::SLArray, s::Size) = similar_type(a, s)(Tuple(a)). StaticArrays no longer makes a copy by default when reshaping.

chriselrod commented 3 years ago

Thanks, I saw your comment: https://github.com/JuliaArrays/StaticArrays.jl/commit/1c72f5c26fd3bcc4006b38a3c583b43e65756418#commitcomment-54021728 and made the suggested change.

chriselrod commented 3 years ago

I applied the change to the mutable LArrays as well, as I figured not being able to mutate the parent array is less likely to be breaking at the moment than losing labels is.

andreasnoack commented 3 years ago

I'm wondering if this would just work without adding special methods here if https://github.com/JuliaArrays/StaticArrays.jl/issues/943 was fixed

chriselrod commented 3 years ago

Does our code rely on having the labels? I know DiffEq moved broadly towards favoring integer indexing.

The offending vecs were being called inside DiffEq, so I'm not sure if the labels are needed. @ChrisRackauckas ?

I could also try and find out.

mateuszbaran commented 3 years ago

I applied the change to the mutable LArrays as well, as I figured not being able to mutate the parent array is less likely to be breaking at the moment than losing labels is.

LArray isn't a StaticArray, are you sure it's correct?

I'm wondering if this would just work without adding special methods here if JuliaArrays/StaticArrays.jl#943 was fixed

I think fixing that would just make reshaping of SLArray return a SizedArray instead of throwing an error.

chriselrod commented 3 years ago

LArray isn't a StaticArray, are you sure it's correct?

Ha, I didn't actually try it. Yes, that method is probably never called.

I think fixing that would just make reshaping of SLArray return a SizedArray instead of throwing an error.

I don't know offhand, so I'd have to test if code is relying on the labels. Although it would probably be preferable to keep the labels in general.

ChrisRackauckas commented 3 years ago

Does our code rely on having the labels? I know DiffEq moved broadly towards favoring integer indexing.

It does, in the broadcast of the @model macro and then in the named indexing of the simobs return. I think it would be best to fix that and remove LabelledArrays, but it's a bit hard and the names are just easy.