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

don't use a field called items #6

Closed
d-v-b opened this issue Jul 9, 2023 · 11 comments
Closed

don't use a field called items #6

d-v-b opened this issue Jul 9, 2023 · 11 comments

Comments

@d-v-b
Copy link
Collaborator

d-v-b commented Jul 9, 2023

Right now, GroupSpec has an items attribute (dict[str, TItem]). But items is a method on mutablemappings. Using items as an attribute is a) confusing, and b) prevents GroupSpec from inheriting from MutableMapping, because of attribute name overlap. We should change the name of the attribute to something that doesn't shadow a core python method.

@d-v-b
Copy link
Collaborator Author

d-v-b commented Jul 9, 2023

possible new names for this attribute: contents, children, elements, stuff...

@clbarnes
Copy link
Contributor

I'd vote for children, personally.

@d-v-b
Copy link
Collaborator Author

d-v-b commented Jul 10, 2023

nodes is another option that just sprang to mind....

I think it would probably be good to promote this discussion to a ZEP outlining a standard JSON-serializable representation of a zarr hierarchy. Then we aren't solely responsible for naming decisions like this, and it's easier for other validation tools to simply copy the standard. Since you had an opinion in this issue, you have any objection to joining me on that ZEP? I'd probably get the ball rolling in the next few days.

@clbarnes
Copy link
Contributor

clbarnes commented Jul 10, 2023

It's only a very gentle opinion, but sure!

@clbarnes
Copy link
Contributor

Thinking about it, isn't that serialized hierarchy exactly what's going to be needed for consolidated metadata? Is there already a consolidated metadata spec in zarr 2?

@d-v-b
Copy link
Collaborator Author

d-v-b commented Jul 11, 2023

I couldn't find a proper standard for consolidated metadata. My quick googling turned up this: zarr-developers/zarr-specs#113, which kind of presages this issue.

I think the consolidated metadata uses a flat namespace for keys, which is convenient for mapping onto the zarr-python API but essentially bars the possibility of doing any static typing (in python at least), because the hierarchy levels all get merged together.

The consolidated metadata also uses the names of objects in storage for keys, e.g. .zattrs, which could be brittle if we think of the names of metadata files as an implementation detail that could change in future versions of zarr.

I see consolidated metadata as a solution to the problem of "I want to use the zarr-python API, but I only want to read one JSON document", whereas the representation in pydantic-zarr solves a different problem. I guess it's an open question if an abstract representation of the zarr hieararchy could be flattened to form a useful consolidated metadata document.

@jakirkham
Copy link

jakirkham commented Jul 26, 2023

Adding other options: edges, leaves, nodes, kids, ninos, etc.

@d-v-b
Copy link
Collaborator Author

d-v-b commented Aug 2, 2023

I think nodes wins here.

@clbarnes
Copy link
Contributor

clbarnes commented Aug 3, 2023

The spec refers mainly to "nodes" and when talking about nodes contained by a particular group, "child nodes" or "children". Could anyone incorrectly assume that "nodes" includes all descendants, or possibly the node itself, rather than just direct children?

@d-v-b
Copy link
Collaborator Author

d-v-b commented Aug 18, 2023

Could anyone incorrectly assume that "nodes" includes all descendants, or possibly the node itself, rather than just direct children?

That seems possible.

@krokicki suggested "members", which I think is even better than "nodes". I'm leaning toward "members" unless there's any objections.

@d-v-b d-v-b mentioned this issue Aug 22, 2023
@d-v-b
Copy link
Collaborator Author

d-v-b commented Aug 22, 2023

closed by #12

@d-v-b d-v-b closed this as completed Aug 22, 2023
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

No branches or pull requests

3 participants