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

docs: Create Ibis for dplyr users document #6050

Merged
merged 1 commit into from
Apr 29, 2023

Conversation

amoeba
Copy link
Contributor

@amoeba amoeba commented Apr 20, 2023

Addresses #5922.

@fmichonneau and I paired up on #5922 during the recent Ibis doc-a-thon. To keep the work in scope for the doc-a-thon, we decided to start small and base the content on https://dplyr.tidyverse.org/articles/dplyr.html. @fmichonneau brought that content in and developed Ibis equivalents for each bit. I then extended that by adding a section at the top highlighting major differences and also added in joins and pivoting to make it more comprehensive.

@fmichonneau could you take a look, especially at the content I added?

After that, I think it'd be good to get @ianmcook to review and also a core Ibis dev.

Specific bits of feedback I'm interested in are:

  • Is the size/scope of this enough? The other Ibis for X guides are more comprehensive.
  • Does the section at the top about major differences work? Is it accurate and complete?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@amoeba amoeba changed the title Create Ibis for dplyr users document docs: Create Ibis for dplyr users document Apr 20, 2023
docs/ibis-for-dplyr-users.ipynb Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Show resolved Hide resolved
@cpcloud cpcloud added this to the 6.0 milestone Apr 20, 2023
@cpcloud cpcloud added the docs Documentation related issues or PRs label Apr 20, 2023
@amoeba
Copy link
Contributor Author

amoeba commented Apr 20, 2023

Thanks for having a go-through @cpcloud. I resolved all but two of your comments.

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together @amoeba and @fmichonneau!

I flagged a few typos and some small language nits, but nothing major.

docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
docs/ibis-for-dplyr-users.ipynb Outdated Show resolved Hide resolved
@ianmcook
Copy link
Contributor

ianmcook commented Apr 26, 2023

One gotcha with Ibis that might trip up users coming from dplyr is when a column name collides with the name of a method of the Ibis table object. For example if you have a column named count. In this case you need to use _["count"] instead of _.count. It might be worth mentioning that somewhere.

@amoeba
Copy link
Contributor Author

amoeba commented Apr 27, 2023

Thanks for the review @gforsyth, I took all of your suggestions as-is and left one question for you about NULL ordering.

Thanks also for the review @ianmcook! I took all of your suggestions and left one conversation unresolved. Your point about name collisions on columns is a good one. It didn't feel high-level enough to cover at the top so I put it as a note in the select section.

@amoeba amoeba force-pushed the ibis-for-dplyr-users branch from cff2c13 to b3d82e5 Compare April 27, 2023 00:43
@amoeba
Copy link
Contributor Author

amoeba commented Apr 27, 2023

Thanks all. I've rebased the branch off main and I think all feedback has been incorporated without issue. This is ready for a final review if anyone wants.

@amoeba amoeba requested review from gforsyth and ianmcook April 27, 2023 17:27
@amoeba
Copy link
Contributor Author

amoeba commented Apr 27, 2023

I addressed one last bit of feedback from @ianmcook in 0609126 and this is again ready for review/merge.

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @amoeba and @fmichonneau !

This looks great and we should :shipit:

Can you squash the commits down and format the message using conventional commit style?

Maybe something like docs(intro): create Ibis for dplyr users document

@amoeba amoeba force-pushed the ibis-for-dplyr-users branch from 33c7a6d to 0a96ee4 Compare April 28, 2023 23:17
@amoeba
Copy link
Contributor Author

amoeba commented Apr 28, 2023

Squashed and amended, thanks @gforsyth. See 0a96ee4.

Co-authored-by: François Michonneau <francois.michonneau@gmail.com>
Co-authored-by: Phillip Cloud <417981+cpcloud@users.noreply.github.com>
Co-authored-by: Ian Cook <ianmcook@gmail.com>
Co-authored-by: Gil Forsyth <gil@forsyth.dev>
@cpcloud cpcloud force-pushed the ibis-for-dplyr-users branch from 0a96ee4 to d3f8ca0 Compare April 29, 2023 13:18
@cpcloud
Copy link
Member

cpcloud commented Apr 29, 2023

Fixed up the pre-commit issue! Merging!

@cpcloud cpcloud merged commit e02a6f2 into ibis-project:master Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants