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.

Sign up for GitHub

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

Jump to bottom

add support for symbols as keys #130

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add support for symbols as keys #130

wants to merge 4 commits into from

Conversation

alex-s-gardner
Copy link
Contributor

This adds support for symbol indexing into a ZGroup

@alex-s-gardner alex-s-gardner mentioned this pull request Nov 27, 2023
Open
@coveralls
Copy link

Pull Request Test Coverage Report for Build 7008504781

  • 1 of 3 (33.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 87.067%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ZGroup.jl 1 3 33.33%
Totals Coverage Status
Change from base Build 6484107136: -0.2%
Covered Lines: 754
Relevant Lines: 866
💛 - Coveralls

@alex-s-gardner
Copy link
Contributor Author

Doc errors appear unrelated to this PR

@meggart
Copy link
Collaborator

meggart commented Nov 30, 2023

Thanks a lot for preparing this. I just looked at it and really like the idea to make the interface more DataFrame-like. I got carried away a bit and implemented dot syntax for subsetting as well so that you can use g.a to access array "a" from group g. Autocompletion works as well which should make working with groups much more ergonomic. However, this would technically be breaking because an array name might shadow the name of field of ZGroup so I had to define some accessors to disambiguate.

Could you have a look if this works for you?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7043631258

  • 47 of 56 (83.93%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.09%) to 87.177%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ZGroup.jl 33 42 78.57%
Files with Coverage Reduction New Missed Lines %
src/ZGroup.jl 1 84.16%
Totals Coverage Status
Change from base Build 6484107136: -0.09%
Covered Lines: 775
Relevant Lines: 889
💛 - Coveralls

@coveralls
Copy link

coveralls commented Dec 1, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 7113230225

  • 55 of 55 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.2%) to 88.501%
Totals Coverage Status
Change from base Build 6484107136: 1.2%
Covered Lines: 785
Relevant Lines: 887
💛 - Coveralls

@alex-s-gardner
Copy link
Contributor Author

in the depths of AGU prep right now but will review this asap

alex-s-gardner
alex-s-gardner commented Jan 3, 2024
View reviewed changes
Copy link
Contributor Author

@alex-s-gardner alex-s-gardner left a comment

Choose a reason for hiding this comment

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

This is fantastic!!! I took the changes for a quick spin and everything was intuitive and worked as expected. I see use of "z.metadata" which I think now needs to be removed and a "metadata()" function likely needs to be added.

src/ZArray.jl
@@ -84,18 +84,18 @@ nobytes(z::ZArray{<:String}) = "unknown"
zinfo(z::ZArray) = zinfo(stdout,z)
function zinfo(io::IO,z::ZArray)
ninit = sum(chunkindices(z)) do i
isinitialized(z.storage,z.path,i)
isinitialized(storage(z),path(z),i)
end
allinfos = [
"Type" => "ZArray",
"Data type" => eltype(z),
"Shape" => size(z),
"Chunk Shape" => z.metadata.chunks,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does z.metadata.chunks need to be updated now that we have . inexing

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so. It is only ZGroups that have the new . indexing where we need to be careful, ZArrays would still behave the same

src/ZArray.jl
@@ -84,18 +84,18 @@ nobytes(z::ZArray{<:String}) = "unknown"
zinfo(z::ZArray) = zinfo(stdout,z)
function zinfo(io::IO,z::ZArray)
ninit = sum(chunkindices(z)) do i
isinitialized(z.storage,z.path,i)
isinitialized(storage(z),path(z),i)
end
allinfos = [
"Type" => "ZArray",
"Data type" => eltype(z),
"Shape" => size(z),
"Chunk Shape" => z.metadata.chunks,
"Order" => z.metadata.order,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does z.metadata.order need to be updated now that we have . inexing

src/ZArray.jl
end
allinfos = [
"Type" => "ZArray",
"Data type" => eltype(z),
"Shape" => size(z),
"Chunk Shape" => z.metadata.chunks,
"Order" => z.metadata.order,
"Read-Only" => !z.writeable,
"Read-Only" => !iswriteable(z),
"Compressor" => z.metadata.compressor,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same comment for all occurrences ofz.metadata

@asinghvi17
Copy link
Member

Adding a metadata function is a bit tricky, since one could mean either .zattrs or .zarray by "metadata".

The simplest solution would be to replace all z.metadata with getfield(z, :metadata) but that would make attribute inspection in general very hard. Some function to get .zarray attributes would be best, but I'm not sure what that is.

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

@meggart meggart meggart left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alex-s-gardner @coveralls @meggart @asinghvi17