move attributes to new file, add docs #943

simonbyrne · 2022-05-29T07:15:19Z

As part of my intermittent effort to clean up the code, this moves all the attributes-related stuff to a new file, and adds some docs.

simonbyrne · 2022-05-29T16:08:17Z

@musm @mkitti

mkitti · 2022-05-29T18:29:11Z

Looks pretty good to me.

Should the Dataset, File, and Group related methods end up in attributes? Should we parcel those out to files as well?

Is there a bigger plan for a file tree?

simonbyrne · 2022-05-29T18:51:45Z

Should the Dataset, File, and Group related methods end up in attributes? Should we parcel those out to files as well?

I'm not sure what you mean: can you give an example?

Is there a bigger plan for a file tree?

I'd like to split up the functionality so it is easier to navigate and understand the API. At the moment, it is not even clear what the user-facing APIs are, let alone where they're defined. I originally tried to split them out with #833, but that was too ambitious, so I decided a piecemeal approach might be easier to manage.

This also makes it a bit easier to refactor and identify smaller changes. For example, after looking at this, there are a couple of things we might to consider to refactor (but I'll leave for separate PRs):

make Attributes a subtype of AbstractDict{String, Attribute}
add additional methods to Attributes (e.g. delete!) to make it full-featured (so that the mid-level API is not required)
deprecate "datasets act like attributes" with getindex/setindex! of strings (calling attributes(obj)["name"] isn't that hard).

mkitti · 2022-05-29T20:00:53Z

deprecate "datasets act like attributes" with getindex/setindex! of strings (calling attributes(obj)["name"] isn't that hard).

While I kind of agree, I'm also not sure why this is a problem. Is there any ambiguity that this causes at the moment?

mkitti · 2022-05-29T20:02:11Z

make Attributes a subtype of AbstractDict{String, Attribute}

I think we should target this for 0.17.

mkitti · 2022-05-29T19:55:07Z

src/attributes.jl

+2.45
+```
+"""
+function read_attribute(parent::Union{File,Group,Dataset,Datatype}, name::AbstractString)


How do we decide that read_attribute belongs in attributes.jl and not objects.jl?

If this were an OOP language, this method would belong to the types of parent.

Do we move all the attribute related methods into their own file mainly because these are leaves in the HDF5 tree?

Basically, I'm just trying to figure out the organization principle so we can be consistent going forward.

I'll admit that there aren't any clear rules, but there are a couple of reasons to keep it here:

they all use the attribute (h5a_XXX) C API functions

attributes are an "extra" thing stuck on top of HDF5 (i.e. you can define groups, datasets etc without referring to attributes at all)

in this particular case, it is a method that applies to multiple objects (so if you were to put it with those, it would need to be defined multiple times)

simonbyrne · 2022-05-29T20:08:33Z

While I kind of agree, I'm also not sure why this is a problem. Is there any ambiguity that this causes at the moment?

it's inconsistent with other objects (Files or Groups), which use it for accessing sub-objects.
when reading code, it's a lot easier to understand what attributes(dataset)["name"] does.

(anyway, that was why i wanted to leave it to a new PR)

mkitti · 2022-05-29T20:10:24Z

I originally tried to split them out with #833,

I agree with changes in #833 more than I disagree with them. Using #833 as a practical example, perhaps prioritizing what to split out and why in an issue might be helpful.

simonbyrne · 2022-05-29T20:15:48Z

docs/src/attributes.md

+## Convenience interface
+
+```@docs
+h5readattr
+h5writeattr
+```


I don't know what to call these functions (honestly, these might be another good candidate for deprecation)

mkitti · 2022-05-29T20:22:15Z

While I kind of agree, I'm also not sure why this is a problem. Is there any ambiguity that this causes at the moment?

it's inconsistent with other objects (Files or Groups), which use it for accessing sub-objects.

when reading code, it's a lot easier to understand what attributes(dataset)["name"] does.

(anyway, that was why i wanted to leave it to a new PR)

Is it possible to have an attribute or dataset with the same name in a group?

One technical advantage I see to your approach is that we would likely improve type stability overall by doing this.

I do think attributes(dataset)["name"] is a bit verbose and hard to intuit though.

mkitti · 2022-05-29T20:22:50Z

Overall, I would be in favor of merging this sooner rather than later.

simonbyrne · 2022-05-29T20:47:12Z

Is it possible to have an attribute or dataset with the same name in a group?

Yes:

julia> h5 = h5open("xx.h5", "cw")
🗂️ HDF5.File: (read-write) xx.h5

julia> attributes(h5)["aa"] = [1.0]
1-element Vector{Float64}:
 1.0

julia> h5["aa"] = [2.0]
1-element Vector{Float64}:
 2.0

julia> h5
🗂️ HDF5.File: (read-write) xx.h5
├─ 🏷️ aa
└─ 🔢 aa

mkitti · 2022-05-29T21:11:32Z

Is it possible to have an attribute or dataset with the same name in a group?

Yes:

I'm convinced. The interface needs to be deprecated.

mkitti · 2022-05-29T21:12:36Z

Let's set a deadline of May 4th to merge this. I just want to give it more time because of the US holiday so everyone can see it.

musm · 2022-05-31T15:11:35Z

src/attributes.jl

+    end
+end
+Base.cconvert(::Type{API.hid_t}, attr::Attribute) = attr
+Base.unsafe_convert(::Type{API.hid_t}, attr::Attribute) = attr.id


musm · 2022-05-31T16:06:02Z

Looks great to me, I'm also in strong approval of all the suggestions in

make Attributes a subtype of AbstractDict{String, Attribute}

add additional methods to Attributes (e.g. delete!) to make it full-featured (so that the mid-level API is not required)

deprecate "datasets act like attributes" with getindex/setindex! of strings (calling attributes(obj)["name"] isn't that hard).

On the last bullet, I agree. It's not very clear why dataset act like attributes and it's certainly far more descriptive to make users call attributes on the dataset if that's what they want to work with.

simonbyrne · 2022-06-02T05:50:19Z

After looking at this a bit more, I thing one other change that may make sense is that getindex(::Attribute, ::String) should actually read the attribute value, rather than returning an Attribute object (since there is no way to read only part of an attribute). This however would be a breaking change.

simonbyrne · 2022-06-02T05:53:08Z

This appears to be what h5py does: https://docs.h5py.org/en/stable/quick.html#attributes

mkitti · 2022-06-02T12:18:57Z

getindex(::Attribute, ::String)

Do you mean getindex(::Attributes, ::String)?

mkitti · 2022-06-02T12:32:16Z

Maybe we should have a attrs that returns a AbstractDict{String, Any} and acts like the h5py interface.

Meanwhile, perhaps we should parameterize attribute by its Julia type and have it work like a Ref.

mkitti · 2022-06-02T12:33:09Z

I would also be interested in comparing to the Zarr.jl as well.

move attributes to new file, add docs

ec9efb6

mkitti reviewed May 29, 2022

View reviewed changes

simonbyrne commented May 29, 2022

View reviewed changes

mkitti approved these changes May 29, 2022

View reviewed changes

simonbyrne mentioned this pull request May 29, 2022

Split up and rearrange files #945

Closed

9 tasks

musm reviewed May 31, 2022

View reviewed changes

musm merged commit 8338e09 into JuliaIO:master May 31, 2022

simonbyrne deleted the sb/attributes branch May 31, 2022 17:34

simonbyrne mentioned this pull request Jun 4, 2022

Define AttributeDict #948

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move attributes to new file, add docs #943

move attributes to new file, add docs #943

simonbyrne commented May 29, 2022

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022 •

edited

Loading

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022

mkitti commented May 29, 2022

mkitti May 29, 2022

simonbyrne May 29, 2022

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022

simonbyrne May 29, 2022

mkitti commented May 29, 2022

mkitti commented May 29, 2022 •

edited

Loading

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022

mkitti commented May 29, 2022

musm May 31, 2022

musm commented May 31, 2022

simonbyrne commented Jun 2, 2022 •

edited

Loading

simonbyrne commented Jun 2, 2022

mkitti commented Jun 2, 2022

mkitti commented Jun 2, 2022

mkitti commented Jun 2, 2022

move attributes to new file, add docs #943

move attributes to new file, add docs #943

Conversation

simonbyrne commented May 29, 2022

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022 • edited Loading

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022

mkitti commented May 29, 2022

mkitti May 29, 2022

Choose a reason for hiding this comment

simonbyrne May 29, 2022

Choose a reason for hiding this comment

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022

simonbyrne May 29, 2022

Choose a reason for hiding this comment

mkitti commented May 29, 2022

mkitti commented May 29, 2022 • edited Loading

simonbyrne commented May 29, 2022

mkitti commented May 29, 2022

mkitti commented May 29, 2022

musm May 31, 2022

Choose a reason for hiding this comment

musm commented May 31, 2022

simonbyrne commented Jun 2, 2022 • edited Loading

simonbyrne commented Jun 2, 2022

mkitti commented Jun 2, 2022

mkitti commented Jun 2, 2022

mkitti commented Jun 2, 2022

mkitti commented May 29, 2022 •

edited

Loading

mkitti commented May 29, 2022 •

edited

Loading

simonbyrne commented Jun 2, 2022 •

edited

Loading