Skip to content

add abstract types for views and permuted arrays#255

Merged
rafaqz merged 8 commits into
JuliaIO:mainfrom
tiemvanderdeure:abstract_types
May 31, 2025
Merged

add abstract types for views and permuted arrays#255
rafaqz merged 8 commits into
JuliaIO:mainfrom
tiemvanderdeure:abstract_types

Conversation

@tiemvanderdeure

@tiemvanderdeure tiemvanderdeure commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

Per @lupemba 's excellent suggesion in JuliaGeo/NCDatasets.jl#274 (comment), this PR implements an AbstractPermutedDiskArray and AbstractSubDiskArray

@rafaqz

rafaqz commented Apr 10, 2025

Copy link
Copy Markdown
Collaborator

Maybe ReshapedDiskArray as well for completeness? Not sure if Padded/Rechunked etc also need abstrac types

@lupemba lupemba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tried using it to implement to solve JuliaGeo/CommonDataModel.jl#32 (comment) and here are my comments

Comment thread src/subarray.jl
Comment thread src/subarray.jl Outdated
Comment thread src/subarray.jl Outdated
Comment thread src/permute.jl Outdated
Base.parent(a::AbstractPermutedDiskArray) = a.a.parent
permuteddimsarray(a) = a.a
Base.size(a::AbstractPermutedDiskArray) = size(permuteddimsarray(a))
Base.parent(a::AbstractPermutedDiskArray) = parent(permuteddimsarray(a))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should merge my other PR first, it actually changes this so there is no separate array. Maybe thats a problem?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Don't think that's a problem at all! The permuteddimsarray was only necessary to get the perm and iperm anyway.

@rafaqz

rafaqz commented May 6, 2025

Copy link
Copy Markdown
Collaborator

@tiemvanderdeure I fixed the merge with #249, it definately makes the abstract type less useful

@lupemba I'm not sure if this will affect you, or if you are subtyping AbstractPermutedDiskArray

@lupemba

lupemba commented May 6, 2025

Copy link
Copy Markdown
Contributor

@lupemba I'm not sure if this will affect you, or if you are subtyping AbstractPermutedDiskArray

I have left out permuted arrays from my PR since CommonDataModel doesn't have specific methods for that yet. So it is not needed right now.

I still think we should consider adding a support for permuted arrays in CommonDataModel before we make a breaking release.

@lupemba

lupemba commented May 28, 2025

Copy link
Copy Markdown
Contributor

@tiemvanderdeure
Do you have time to look at this PR. It would be great to get it finished so I can continue with JuliaGeo/CommonDataModel.jl#35

@tiemvanderdeure

Copy link
Copy Markdown
Contributor Author

I think it's basically good to go. Let's see if tests pass now.

Comment thread src/permute.jl
end
function DiskArrays.readblock!(a::PermutedDiskArray, aout, i::OrdinalRange...)
function DiskArrays.readblock!(a::AbstractPermutedDiskArray, aout, i::OrdinalRange...)
iperm = _getiperm(a)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is now an internal method in an Abstract typed function... Should we remove the underscore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Remove which underscore? You mean in _getiperm and _getperm

Comment thread src/permute.jl Outdated
@rafaqz rafaqz merged commit 6522ba3 into JuliaIO:main May 31, 2025
12 checks passed
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.

3 participants