Suppor dimensional metadata #62

Tokazama · 2023-07-07T03:28:32Z

The core metadata methods (metadata, metadata!, etc.) provide an intuitive method of communicating that a key-value pair provides some information about the instance of the object it is attached to. The column metadata methods similarly provide an intuitive method of communicating the a key-value pair provides some information about the column of a particular object. However, data with multiple dimensions (array, mesh, coordinates, etc.) often have metadata stored per dimension.

This PR provides support for metadata corresponding to specific dimensions. New method implemented here follow the name pattern of column metadata methods (colmetadata to dimmetadata).

quinnj · 2023-07-10T08:44:37Z

This seems fine to me? @bkamins any concerns?

test/runtests.jl

bkamins · 2023-07-10T13:49:25Z

I think it is OK to add. I assume @Tokazama has some concrete use case (i.e. I would avoid adding functionalities that will not be used in practice). I have went through the code and left some small comments.

Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>

Tokazama · 2023-07-11T12:47:32Z

I think it is OK to add. I assume @Tokazama has some concrete use case (i.e. I would avoid adding functionalities that will not be used in practice). I have went through the code and left some small comments.

Besides the use cases in the original post I have images that have metadata per dimension in the form of name, per dim machine acquisition information, and additional metadata along axes (similar to colmetadata).

codecov · 2023-07-12T12:23:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b131356) 96.36% compared to head (e1b08c5) 96.77%.
Report is 1 commits behind head on main.

❗ Current head e1b08c5 differs from pull request most recent head 6a0af43. Consider uploading reports for the commit 6a0af43 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
+ Coverage   96.36%   96.77%   +0.41%     
==========================================
  Files           1        1              
  Lines          55       62       +7     
==========================================
+ Hits           53       60       +7     
  Misses          2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bkamins · 2023-07-15T07:12:29Z

@quinnj - all looks good. OK to merge? (I would make a release next)

Tokazama · 2023-07-18T14:45:39Z

Anything needed on my end?

bkamins · 2023-07-18T15:42:15Z

Needs a second approval (which might require some changes, but most likely not) before being merged and released.

bkamins · 2023-08-05T21:09:55Z

@nalimilan - do you have an opinion on this PR?

nalimilan

Makes sense.

src/DataAPI.jl

test/runtests.jl

Tokazama · 2023-08-07T01:19:10Z

Why not write 1 instead of using a variable? That sounds clearer to me.

The column metadata tests use :col as the argument for which column is being accessed. I thought using the variable dim instead of just 1 would make it easier to quickly read and understand what exactly was being tested.

nalimilan · 2023-08-14T14:26:38Z

src/DataAPI.jl

+    dimmetadatasupport(T::Type, dim::Int)
+
+Return a `NamedTuple{(:read, :write), Tuple{Bool, Bool}}` indicating whether
+values of type `T` support metadata correspond to dimension `dim`.


Sorry, I missed this substantial point: colmetadatasupport takes a single argument, i.e. it asks whether column metadata is supported at all, without considering a particular column. Shouldn't we do the same here? Once a type supports dimension metadata, it can return nothing for dimensions that do not have any metadata.

I think this is could be OK as is proposed here. colmetadatasupport takes one argument, because there is exactly one dimension always.
But what @nalimilan is also OK - it depends on what @Tokazama thinks is better (i.e. do we imagine that only selected dimensions would have metadata?).

What do you mean by "there is exactly one dimension always"? :-)

OK I get it now, you mean that columns are the second dimension so colmetadatasupport(T) is similar to dimmetadatasupport(T, 2).

I still think there's a problem though: dimmetadata(x) returns a tuple of metadata for each dimension. But how can you check whether you can call dimmetadata(x) without getting an error? For that you would need to be able to call dimmetadatasupport(T).

src/DataAPI.jl

nalimilan · 2023-08-14T14:39:54Z

Question on the intended use: is this PR is about storing information about a dimension itself (e.g. its name or unit), or about its elements (e.g. row names for dimension 1), or both? If the second option, I wonder how this interacts with the existence of column metadata, as columns are usually considered as dimension 2.

Also, how does this relate to AxisArrays, NamedArrays, NamedDims, etc.?

bkamins · 2023-08-14T14:48:19Z

I wonder how this interacts with the existence of column metadata, as columns are usually considered as dimension 2.

I assumed this is completely detached. I.e. colmetadata is for objects that implement Tables.jl interface. And this PR is for array-like objects. Note that array-like objects can be re-interpreted as tables in many different ways.

Tokazama · 2023-08-14T15:56:20Z

The way to support metadata per element along a dimension with this would be a specific "indices" or "axis" key whose value can be used somewhat like a tables column metadata.

nalimilan · 2023-08-14T17:14:07Z

I assumed this is completely detached. I.e. colmetadata is for objects that implement Tables.jl interface. And this PR is for array-like objects. Note that array-like objects can be re-interpreted as tables in many different ways.

@bkamins Sure. But e.g. for a DataFrame we could imagine implementing dimmetadata at some point given that we define ndims, axes, getindex... So I wanted to make sure we consider whether that would make sense.

The way to support metadata per element along a dimension with this would be a specific "indices" or "axis" key whose value can be used somewhat like a tables column metadata.

@Tokazama I guess that would work. Then you mean that the intended use case for this PR is both metadata about a dimension itself and metadata about its elements (rows, columns...), right?

Tokazama · 2023-08-14T23:03:06Z

I assumed this is completely detached. I.e. colmetadata is for objects that implement Tables.jl interface. And this PR is for array-like objects. Note that array-like objects can be re-interpreted as tables in many different ways.

@bkamins Sure. But e.g. for a DataFrame we could imagine implementing dimmetadata at some point given that we define ndims, axes, getindex... So I wanted to make sure we consider whether that would make sense.

The way to support metadata per element along a dimension with this would be a specific "indices" or "axis" key whose value can be used somewhat like a tables column metadata.

@Tokazama I guess that would work. Then you mean that the intended use case for this PR is both metadata about a dimension itself and metadata about its elements (rows, columns...), right?

Exactly! I figured the alternative was having an additional metadata set of methods (e.g., axesmetadata) which seemed a bit over the top since one can still accomplish similar functionality with the dim methods here. The overlap with column metadata could also be seen as redundant but tables have consistently proven to be very unique since each column can be conceptualized as its own entity.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

src/DataAPI.jl

nalimilan · 2023-08-15T08:37:51Z

src/DataAPI.jl

+function dimmetadatakeys(x, dim::Int)
+    dimmetadatasupport(typeof(x), dim).read || return ()
+    throw(MethodError(dimmetadatakeys, (x, dim)))
+end
+function dimmetadatakeys(x)
+    Dict(dim => dimmetadatakeys(x, dim) for dim in 1:ndims(x))
+end


I don't remember the discussions offhand, but for colmetadatakeys we decided not to provide any fallback definition, in particular not to return () when metadata is not supported. Better be consistent for dimmetadatakeys, otherwise the API will be confusing (or, again, also adapt colmetadatakeys if necessary).

The reason I had the fall back is because the docs still say "If x does not support column metadata return ()". Should the docs for both of these be changed then?

That sentence is in the paragraph regarding the case when col is omitted. When it's passed, the docstring says that we throw an error. We cannot change this for colmetadatakeys anyway as it would be breaking, so better make dimmetadatakeys consistent with that.

src/DataAPI.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Tokazama · 2023-12-21T17:38:51Z

@nalimilan, is this okay now?

src/DataAPI.jl

nalimilan · 2023-12-22T08:51:31Z

src/DataAPI.jl

+    dimmetadatasupport(T::Type, dim::Int)
+
+Return a `NamedTuple{(:read, :write), Tuple{Bool, Bool}}` indicating whether
+values of type `T` support metadata correspond to dimension `dim`.


OK I get it now, you mean that columns are the second dimension so colmetadatasupport(T) is similar to dimmetadatasupport(T, 2).

I still think there's a problem though: dimmetadata(x) returns a tuple of metadata for each dimension. But how can you check whether you can call dimmetadata(x) without getting an error? For that you would need to be able to call dimmetadatasupport(T).

src/DataAPI.jl

nalimilan · 2023-12-23T20:13:37Z

test/runtests.jl

+    if DataAPI.dimmetadatasupport(TestMeta, dim).read
+        return keys(x.dims[dim])
+    else
+        return ()
+    end


The docstring says we should throw an error when dim doesn't exist. Can you test this behavior?
@bkamins We should probably also change colmetadatakeys?

Suggested change

if DataAPI.dimmetadatasupport(TestMeta, dim).read

return keys(x.dims[dim])

else

return ()

end

return keys(x.dims[dim])

test/runtests.jl

src/DataAPI.jl

nalimilan · 2023-12-23T21:06:42Z

src/DataAPI.jl

+
+If `dim` is not passed return an iterator of `dim => dimmetadatakeys(x, dim)`
+pairs for all dimensions that have metadata.
+


Let's do the same as for colmetadatakeys. Can you update the implementation accordingly?

Suggested change

If `x` does not support dimension metadata return `()`.

nalimilan · 2023-12-23T21:08:52Z

src/DataAPI.jl

+function dimmetadatakeys(x, dim::Int)
+    dimmetadatasupport(typeof(x), dim).read || return ()
+    throw(MethodError(dimmetadatakeys, (x, dim)))
+end
+function dimmetadatakeys(x)
+    Dict(dim => dimmetadatakeys(x, dim) for dim in 1:ndims(x))
+end


That sentence is in the paragraph regarding the case when col is omitted. When it's passed, the docstring says that we throw an error. We cannot change this for colmetadatakeys anyway as it would be breaking, so better make dimmetadatakeys consistent with that.

nalimilan

Sorry, looks like I missed your last push. I've made suggestions to ensure docstrings have the same structure as existing ones. There's also one decision to make about dimmetadatasupport (see discussion).

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

dimension metadata support

5ae5ab0

bkamins reviewed Jul 10, 2023

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

bkamins reviewed Jul 10, 2023

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

bkamins reviewed Jul 10, 2023

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

Update test/runtests.jl

47daf4c

Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>

Version bump and delete unnecessary line

e1b08c5

Tokazama requested a review from bkamins July 13, 2023 20:28

bkamins approved these changes Jul 14, 2023

View reviewed changes

nalimilan reviewed Aug 6, 2023

View reviewed changes

src/DataAPI.jl Outdated Show resolved Hide resolved

src/DataAPI.jl Outdated Show resolved Hide resolved

test/runtests.jl Outdated Show resolved Hide resolved

Make docs more consistent with colmetadata

19287e0

remove dim

10ca2f0

nalimilan reviewed Aug 14, 2023

View reviewed changes

Tokazama and others added 3 commits August 14, 2023 21:04

Update src/DataAPI.jl

bbe4fc7

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

fix typos

ffbb582

Support dimmetadata and dimmetadatakeys without dim arg

3a0466f

nalimilan reviewed Aug 15, 2023

View reviewed changes

Tokazama and others added 3 commits August 16, 2023 10:25

Update src/DataAPI.jl

1a418e3

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

115962f

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Improve docs for metadata keys for dims and cols

7cb0aef

nalimilan reviewed Dec 23, 2023

View reviewed changes

Tokazama and others added 15 commits December 24, 2023 14:27

Update src/DataAPI.jl

0d5bb5c

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

4a11c37

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update test/runtests.jl

8225a7e

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

2ae2768

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

57e4f8c

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

e68ece2

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

c35f7a1

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

a3ecbcd

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

b519d26

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

ab256ec

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

838047a

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

c0ae557

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/DataAPI.jl

2e343f4

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Add doc to sync metadatakeys and metadatasupport

b01d2c2

Move colmetadatasupport above colmeta functions

6a0af43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppor dimensional metadata #62

Suppor dimensional metadata #62

Tokazama commented Jul 7, 2023

quinnj commented Jul 10, 2023

bkamins commented Jul 10, 2023

Tokazama commented Jul 11, 2023

codecov bot commented Jul 12, 2023 •

edited

Loading

bkamins commented Jul 15, 2023

Tokazama commented Jul 18, 2023

bkamins commented Jul 18, 2023

bkamins commented Aug 5, 2023

nalimilan left a comment

Tokazama commented Aug 7, 2023

nalimilan Aug 14, 2023

bkamins Aug 14, 2023

nalimilan Aug 14, 2023

nalimilan Dec 22, 2023

nalimilan commented Aug 14, 2023

bkamins commented Aug 14, 2023

Tokazama commented Aug 14, 2023

nalimilan commented Aug 14, 2023

Tokazama commented Aug 14, 2023

nalimilan Aug 15, 2023

Tokazama Aug 16, 2023

nalimilan Dec 23, 2023

Tokazama commented Dec 21, 2023

nalimilan Dec 22, 2023

nalimilan Dec 23, 2023

nalimilan Dec 23, 2023

nalimilan Dec 23, 2023

nalimilan left a comment •

edited

Loading


		If `dim` is not passed return an iterator of `dim => dimmetadatakeys(x, dim)`
		pairs for all dimensions that have metadata.

Suppor dimensional metadata #62

Are you sure you want to change the base?

Suppor dimensional metadata #62

Conversation

Tokazama commented Jul 7, 2023

quinnj commented Jul 10, 2023

bkamins commented Jul 10, 2023

Tokazama commented Jul 11, 2023

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

bkamins commented Jul 15, 2023

Tokazama commented Jul 18, 2023

bkamins commented Jul 18, 2023

bkamins commented Aug 5, 2023

nalimilan left a comment

Choose a reason for hiding this comment

Tokazama commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Aug 14, 2023

bkamins commented Aug 14, 2023

Tokazama commented Aug 14, 2023

nalimilan commented Aug 14, 2023

Tokazama commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tokazama commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2023 •

edited

Loading

nalimilan left a comment •

edited

Loading