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

Metadata: follow-up notes #3168

Closed
bkamins opened this issue Sep 20, 2022 · 4 comments · Fixed by #3169
Closed

Metadata: follow-up notes #3168

bkamins opened this issue Sep 20, 2022 · 4 comments · Fixed by #3169
Labels
Milestone

Comments

@bkamins
Copy link
Member

bkamins commented Sep 20, 2022

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 bkamins added the doc label Sep 20, 2022
@bkamins bkamins added this to the 1.4 milestone Sep 20, 2022
@eirikbrandsaas
Copy link
Contributor

eirikbrandsaas commented Sep 20, 2022

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)

@nalimilan
Copy link
Member

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".

@eirikbrandsaas
Copy link
Contributor

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

@bkamins
Copy link
Member Author

bkamins commented Sep 21, 2022

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

This is the point of having metadatakeys and colmetadatakeys functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants