Skip to content

Conversation

@rafaqz
Copy link
Collaborator

@rafaqz rafaqz commented Apr 1, 2025

Closes #240

@rafaqz
Copy link
Collaborator Author

rafaqz commented Apr 2, 2025

Ok so to remove the recursive PermuteDimsArray I moved type parameters perm and iperm from PermutedDimsArray to PermutedDiskArray. But then I had to add a ConstructionBase dependency so we can use Flatten.jl/Accessors.jl etc on it (so Rasters and other things don't break). ConstructionBase.jl is tiny and will nearly always be loaded anyway.

Then there were some issues with parent access after changes to PermutedDiskArray, so I went ahead and swapped everything to use parent(a).

@rafaqz rafaqz requested a review from meggart April 3, 2025 10:28
Copy link
Collaborator

@meggart meggart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Would you consider this change to be breaking?

PermutedDiskArray{T,N,perm,iperm,A}(a)

ConstructionBase.constructorof(::Type{<:PermutedDiskArray{<:Any,N,perm,iperm}}) where {N,perm,iperm} =
PermutedDiskArrayConstructor{N,perm,iperm}()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding the ConstructionBase dependecy is a good addition. I think there are several places in DiskArrays where we added unnecessary fields to types that might have lived in the type parameter only, so we could slowly replace these as well.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Apr 21, 2025

I guess it could be considered technically breaking, But I imagine the scope of the break is fairly limited. The PermutedDimsArray was essentially useless anyway as it was scalar indexing, so its not likely to break any real workflows.

@tiemvanderdeure
Copy link
Contributor

I guess DD is going to require this to make permutedims work, right? So then it really wouldn't be ideal to release a 0.5 - it would cause compatibility issues with the whole ecosystem until everyone updates.

A PermutedDimsArray of a DiskArray was basically unusable anyway.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Apr 23, 2025

Absolutely, I think we should just st bump a patch version

@rafaqz rafaqz merged commit 80274cd into main May 6, 2025
10 checks passed
@mtfishman
Copy link
Contributor

mtfishman commented Sep 4, 2025

I hadn't noticed this change until now, but this broke some code in one of my packages where I define my own AbstractDiskArray subtype, and I have code that assumed that PermuteDimsArray(::MyDiskArray, perm) returns a PermutedDimsArray (which I then convert to my own PermutedDiskArray-like type, similar to the previous logic in DiskArrays.jl). While I understand the motivation behind it, I think this behavior is pretty surprising, what about defining:

permuteddims(a::AbstractArray, perm) = PermutedDimsArray(a, perm)
permuteddims(a::AbstractDiskArray, perm) = PermutedDiskArray(a, perm)

and then use permuteddims inside of DiskArrays.jl in places where you might return a PermutedDiskArray? That's what I do in some of my own packages. Of course it would be ideal if that kind of design was in Base, though in the meantime we could make a small interface package that defines permuteddims that is shared across packages.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Sep 6, 2025

The problem is the interface to other packages and users that don't know about this method. And that anyone ever calling PermuteDimsArrray on a disk array is fundamentally broken (it causes scalar indexing) and shouldnt be allowed to happen in any case.

We really need a method in Base Julia, as you have read in my issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle PermutedDimsArray

4 participants