Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

static interface for permutedims #806

Open
tpapp opened this issue Jul 7, 2020 · 11 comments
Open

static interface for permutedims #806

tpapp opened this issue Jul 7, 2020 · 11 comments
Labels
arrays AbstractArray interface and array operations feature features and feature requests

Comments

@tpapp
Copy link
Contributor

tpapp commented Jul 7, 2020

I would like to contribute an implementation for permutedims, but first I would like to ask for suggestions about the interface.

In particular, it would be ideal if one could encode the permutation statically. I could imagine a

struct StaticPermutation{N}
    permuation::NTuple{N,Int}
    ## optionally: constructor that verifies validity
end

type and an interface like

permutedims(sarray, StaticPermutation(2, 3, 1))

but I am interested in suggestions if there is a better solution.

@mateuszbaran
Copy link
Collaborator

Good idea, although for full type stability you need to encode the entire permutation in the type, such as

struct StaticPermutation{Permutation<:Tuple}
end

and then have something like permutedims(sarray, StaticPermutation{Tuple{2, 3, 1}}()). Of course we could have a nicer constructor for permutations.

@c42f
Copy link
Member

c42f commented Jul 8, 2020

Agreed, we should have StaticPermutation(2, 3, 1) encode the permutation purely in the type of StaticPermutation so that the output of permutedims(sarray, StaticPermutation(2, 3, 1)) would always have a known Size. Otherwise, passing StaticPermutation would be equivalent to passing a normal NTuple (or SVector) as the permutation to permutedims.

It may also make logical sense to have the permutation as a kind of StaticVector, sorta like

struct StaticPermutation{N,Permutation<:NTuple{N,Int}} <: StaticVector{N,Int}
end

but the extra N parameter is annoying (can we avoid it?)

@c42f c42f added feature features and feature requests arrays AbstractArray interface and array operations labels Jul 8, 2020
@tkf
Copy link
Member

tkf commented Jul 8, 2020

I think this is where StaticNumbers.jl is useful. Something like permutedims(sarray, (static(2), static(3), static(1))) is cleaner and more extensible. You can construct and manipulate the permutation as if everything is done in the value domain rather than in the type domain (e.g., isperm((static(2), static(3), static(1))) works and const props without any specific methods).

OTOH, using Tuple{2, 3, 1} feels like a hack (although I know StaticArray type has been using it since forever). I understand that it is tempting to use Tuple whenever you need "vararag" type parameters. But I don't think we should be using Tuple with non-types as parameters, especially when there are other better ways to do it.

@c42f
Copy link
Member

c42f commented Jul 8, 2020

Great observation, I'd forgotten about StaticNumbers.jl.

OTOH, using Tuple{2, 3, 1} feels like a hack (although I know StaticArray type has been using it since forever).

Completely agreed about it being a hack. This hack was used in the old FixedSizeArrays.jl but StaticArrays didn't start off this way. Unfortunately it's also very useful because it enables type constructors like

const SVector{S, T} = SArray{Tuple{S}, T, 1, S}

Without being able to do this we previously had SVector a completely separate type from SArray.

@tkf
Copy link
Member

tkf commented Jul 8, 2020

How about

const SVector{S, T} = SArray{1, Tuple{StaticInteger{S}}, T, S}

with

struct SArray{N, S <: NTuple{N,StaticInteger}, T, L}
    data::NTuple{L,T}
    size::S
end

?

@c42f
Copy link
Member

c42f commented Jul 8, 2020

That's 2.0 material for sure (or even material for a separate StaticArrays2.jl) because it would break basically everything.

But other than that 💯 — I think you're right and discussing this deserves an issue of its own.

One thing which has long bothered me is the inability to use Size quite the way I want to when mixing static and dynamic dimensions — I've ended up reimplementing things in the type and value domains separately which is really dissatisfying. The right way out of that is to use a mixed Tuple of StaticInteger and Int, not a Size parameterized on a Tuple of Int and Dynamic(). (Come to think of it, maybe I said this somewhere else...)

@tkf
Copy link
Member

tkf commented Jul 8, 2020

because it would break basically everything

Yeah, I agree that's the hardest problem.

or even material for a separate StaticArrays2.jl

I'm actually OK with StaticArrays2.jl but another path might be to change only the UUID JuliaLang/julia#33047

@martinholters
Copy link
Collaborator

Would you keep maintaining StaticArrays and StaticArrays2? If no, people would be forced to migrate to StaticArrays2 which doesn't seem to be much better than migrating to StaticArrays v2.0, does it? Or would that be to allow different dependencies of the same project (or the project itself) to mix StaticArrays and StaticArrays2?

@c42f
Copy link
Member

c42f commented Jul 8, 2020

Or would that be to allow different dependencies of the same project (or the project itself) to mix StaticArrays and StaticArrays2?

Yes, exactly - the thought would be to decouple the upgrade process between packages across the ecosystem, as the concrete type name would be spelled differently and this would just break all the things. But a big split is not great; perhaps we can have a transition plan where we transition users to never spelling out the concrete type names, as mentioned in #807 (comment).

@tpapp
Copy link
Contributor Author

tpapp commented Jul 8, 2020

While I fully agree with Tuple{1,2,3} being a hack, replacing it in this package is beyond the scope of this issue.

If that's OK, I would just submit a PR implementing this feature following the suggestion of @mateuszbaran, and this can be changed later on when the whole package is refactored.

@c42f
Copy link
Member

c42f commented Jul 8, 2020

Of course @tpapp we don't expect you to solve the long-standing design issues for one small feature.

If we do take a StaticNumbers dependency we can easily deprecate StaticPermutation(xs...) to mean map(static, xs) in the future and change the signature to the ever-so-charming

Base.permutedims(sa::StaticArray, perm::NTuple{N,StaticInteger}) where {N}

so that usage would become permutedims(sarray, static.((2,3,1))).

This suggest we don't want StaticPermutation <: StaticVector, and also to discourage users from writing the concrete type of StaticPermutation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays AbstractArray interface and array operations feature features and feature requests
Projects
None yet
Development

No branches or pull requests

5 participants