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

Make AccessCountDiskArray <: AbstractDiskArray + fix zip bug #193

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

charleskawczynski
Copy link
Contributor

@charleskawczynski charleskawczynski commented Sep 19, 2024

I'm hoping that this will alleviate or close #175.

@charleskawczynski
Copy link
Contributor Author

Oh, wait, @rafaqz, can we just move the TestTypes module outside of source? Why would DiskArrays ship that anyway?

@felixcremer
Copy link
Contributor

We use TestTypes in higher level packages like YAXArrays to ensure that code is IO efficient.

@charleskawczynski
Copy link
Contributor Author

We use TestTypes in higher level packages like YAXArrays to ensure that code is IO efficient.

Couldn't that just be shipped as a library instead?

@felixcremer
Copy link
Contributor

There might be a way to put this in an extension on Test, but then we would need to get the types out of the extension.

@charleskawczynski
Copy link
Contributor Author

There might be a way to put this in an extension on Test, but then we would need to get the types out of the extension.

Yeah, an extension would work, or even just make a DiskArraysTestTypes package in this repo, and then just depend on that where needed. I don't think that this is a blocker for this PR, but I think moving it to a separate package is a cleaner solution entirely.

src/util/testtypes.jl Outdated Show resolved Hide resolved
Co-authored-by: Rafael Schouten <rafaelschouten@gmail.com>
@charleskawczynski
Copy link
Contributor Author

charleskawczynski commented Sep 20, 2024

I think I found a bug!

function Base.zip(A1::AbstractDiskArray, A2::AbstractArray, As::AbstractArray...)
    return DiskZip(A1, A1, As...)
end

should be

function Base.zip(A1::AbstractDiskArray, A2::AbstractArray, As::AbstractArray...)
    return DiskZip(A1, A2, As...)
end

! This is probably pretty confusing since it was correctly implemented in implement_zip! I also removed the DiskZip(As...) method because we already check dimensions in DiskZip(As::AbstractArray...).

@charleskawczynski charleskawczynski changed the title Make AccessCountDiskArray a subtype of AbstractDiskArray Make AccessCountDiskArray <: AbstractDiskArray + fix zip bug Sep 20, 2024
@rafaqz rafaqz merged commit 1cc1d91 into JuliaIO:main Sep 20, 2024
10 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.

zip triggers lots of invalidations
3 participants