search

scicloj / tablecloth

Dataset manipulation library built on the top of tech.ml.dataset
https://scicloj.github.io/tablecloth
MIT License
305 stars 27 forks source link

Additions and changes to initial part of Group-by section #115

Closed mars0i closed 1 year ago

mars0i commented 1 year ago

Added paragraph explaining that most operations apply to sub-datasets, and gave example to illustrate the idea.

Moved up two lines that relate to this point.

Added explanation of effect of grouping parameters on selection of rows for sub-datasets. I couldn't figure out how to describe the map from group names to indexes in the same way, so I made this item last, and gave it a different explanation.

Fixed one minor typo in first line of Group-by section (added "s" to "pack").

Fixed minor typos in line about grouped? meta tag.

mars0i commented 1 year ago

The commit message contains the same text twice. Sorry about that. I'm sure there's a way to fix it, but I couldn't figure it out, and it didn't seem worth a lot of time.

genmeblog commented 1 year ago

Cool! Thanks. I'll read it tomorrow (midnight here) and let you know if it's ok.

mars0i commented 1 year ago

Great. Also, I was thinking of suggesting additions to the group-by docstring, analogous to what I did TMD's group-by (https://github.com/techascent/tech.ml.dataset/pull/375), but the docstring for TC's group-by would be different, because its behavior is more complex. I thought I'd wait and see what you thought of the index.Rmd changes. I can submit a separate PR for the docstring if that seems worthwhile.

genmeblog commented 1 year ago

Do you want to work more on this topic? Or should I merge and deploy?

mars0i commented 1 year ago

Thanks @genmeblog. I'm fine with you merging it. (I saw some typos in a keyword name in another part of the doc, but they're completely unrelated, so I think I should submit a different PR for that.)

mars0i commented 1 year ago

I decided it was silly to leave the typos unfixed. There were two instances of :ungroup in the text that should be :ungroup?. That's now corrected. It concerns aggregate, but applied to grouped datasets, so it's related to the other parts of this PR. I hope you don't mind--I can roll it back if you want.

genmeblog commented 1 year ago

Great, thanks! I don't mind, of course. We can do doc updates in batches. It doesn't matter if they are related to one topic or many. The most important is to have it better and better.

genmeblog commented 1 year ago

No need to squash. I will be merging today and releasing new version (there are some other changes waiting).