Skip to content

add support for symbols as keys#130

Open
alex-s-gardner wants to merge 4 commits into
JuliaIO:mainfrom
alex-s-gardner:master
Open

add support for symbols as keys#130
alex-s-gardner wants to merge 4 commits into
JuliaIO:mainfrom
alex-s-gardner:master

Conversation

@alex-s-gardner

Copy link
Copy Markdown
Contributor

This adds support for symbol indexing into a ZGroup

@coveralls

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

Doc errors appear unrelated to this PR

@meggart

meggart commented Nov 30, 2023

Copy link
Copy Markdown
Collaborator

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
Copy Markdown

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

coveralls commented Dec 1, 2023

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

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

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

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.

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.

Comment thread src/ZArray.jl
"Type" => "ZArray",
"Data type" => eltype(z),
"Shape" => size(z),
"Chunk Shape" => z.metadata.chunks,

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.

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

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.

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

Comment thread src/ZArray.jl
"Data type" => eltype(z),
"Shape" => size(z),
"Chunk Shape" => z.metadata.chunks,
"Order" => z.metadata.order,

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.

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

Comment thread src/ZArray.jl
"Order" => z.metadata.order,
"Read-Only" => !z.writeable,
"Read-Only" => !iswriteable(z),
"Compressor" => z.metadata.compressor,

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.

same comment for all occurrences ofz.metadata

@asinghvi17

Copy link
Copy Markdown
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.

@felixcremer

Copy link
Copy Markdown
Member

What is the status of this? Is it worth to revive?

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.

5 participants