DataFrames.jl icon indicating copy to clipboard operation
DataFrames.jl copied to clipboard

Metadata: follow-up notes

Open bkamins opened this issue 1 year ago • 4 comments

These are suggestions @eirikbrandsaas from Slack, with my responses:

Notably metadata is not supported for GroupedDataFrame

"does not expose" means that even if data frame underlying GroupedDataFrame has metadata then if you query GroupedDataFrame for metadata it will return that no metadata is associated to it. That is why it says "does not expose".

@eirikbrandsaas - how would you phrase this more clearly?

I found it a little confusing how metadata!(...,style=:note) and metadata(...,style=true)

@nalimilan - what do you think?

For me keeping style would be acceptable. Especially that I do not expect that many users will call metadata with style=true. This is mostly for internal purposes of metadata propagation. Also in general I expect that this low-level API will not be used much. When TableMetadataTools.jl package is created I plan to put there simpler functions that users would work with in typical scenarios.

to a red warning box or something

I would keep blue for consistency. Especially that this is a warning added to be on the safe side. I do not expect that we will change the rules, but they might change so it is better to warn users about it. The point is that we want users to use this feature without fearing that in 3 months all will change.

It would be GREAT if there was an example shown where you use your chess elo-ratings dataframe

This will be added when more packages support metadata and TableMetadataTools.jl package is created. Now this would be quite cumbersome as at this stage we provide a low-level API only.

bkamins avatar Sep 20 '22 13:09 bkamins

How about this? (I assume this is right? Or can you add metadata to groupeddataframe?)

"Notably, metadata is not supported for GroupedDataFrame and you can't add, modify, nor view metadata through the GroupedDataFrame itself, only through its parent."

(Edited to follow @nalimilan's suggestion below)

eirikbrandsaas avatar Sep 20 '22 13:09 eirikbrandsaas

I found it a little confusing how metadata!(...,style=:note) and metadata(...,style=true)

@nalimilan - what do you think?

For me keeping style would be acceptable. Especially that I do not expect that many users will call metadata with style=true. This is mostly for internal purposes of metadata propagation. Also in general I expect that this low-level API will not be used much. When TableMetadataTools.jl package is created I plan to put there simpler functions that users would work with in typical scenarios.

I'm not too worried by the confusion either. IMO it would be a problem only if metadata(..., style=:note) was allowed, but I don't see what it could mean. I don't see a good alternative either show_style isn't correct as we don't show anything, get_style isn't super nice... Anyway it's too late as we released DataAPI 1.11. ;-)

The phrasing proposed by @eirikbrandsaas about GroupedDataFrame sounds good to me. Maybe just say "nor" instead of "or", and "its" instead of "it's".

nalimilan avatar Sep 20 '22 14:09 nalimilan

One more comment: How about adding a sentence or two about the correct/proper/recommended way to test if a metadata field exists?

eirikbrandsaas avatar Sep 21 '22 13:09 eirikbrandsaas

key in metadatakeys(df) or key in colmetadatakeys(df, col). I can add it.

This is the point of having metadatakeys and colmetadatakeys functions.

bkamins avatar Sep 21 '22 13:09 bkamins