search

JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
374 stars 55 forks source link

set/getindex with broadcast and for Cartesian indices #180

Closed kellertuer closed 4 years ago

kellertuer commented 4 years ago

I noticed that if you do

using Manifolds, Manopt
M = Sphere(2)
N = PowerManifold(M, NestedPowerRepresentation(), 3)
p = [[1.0,0.0,0.0], [0.0,1.0,0.0], [0.0,0.0,1.0]]

Then with the new mode the following works perfectly fine

p[N,2]
p[N,2] += [1.0,0.0,0.0]

But with broadcast or Cartesian indices

p[N,CartesianIndex(2,)]
p[N,2] .+= [1.0,0.0,0.0]

it does not yet.

mateuszbaran commented 4 years ago

I haven't worked much with Cartesian indices but p[N,CartesianIndex(2,).I...] does work.

mateuszbaran commented 4 years ago

I've opened a PR that makes broadcasting work but I'm not sure if there aren't any potential problems with that. Non-standard broadcasting sometimes has issues.

kellertuer commented 4 years ago

I haven't worked much with Cartesian indices but p[N,CartesianIndex(2,).I...] does work.

That is exactly what I am currently doing, or more precidely from an I = CartesianIndex(2,) I do Tuple(I).... On the default setindex it also worked directly with CartesianIndex, I think it was converted somewhere.

mateuszbaran commented 4 years ago

Cartesian indices shouldn't work for setindex! either. I've briefly looked at how they are treated in Julia Base and it's slightly convoluted so I'd prefer to leave it for now, especially since there is an easy workaround.

kellertuer commented 4 years ago

Currently in our version it does not, that's right. But yes the workaround exists and is only a few characters longer (Tuple(i)... compared to just I), so yes, we can close this.