Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ggplot histogram query change #754

Merged
merged 2 commits into from
Jul 26, 2023
Merged

ggplot histogram query change #754

merged 2 commits into from
Jul 26, 2023

Conversation

bbeat2782
Copy link

@bbeat2782 bbeat2782 commented Jul 24, 2023

Describe your changes

Changed how histogram binning query works so that it mimics R ggplot behavior.

Issue number

Closes #751

Checklist before requesting a review


📚 Documentation preview 📚: https://jupysql--754.org.readthedocs.build/en/754/

@bbeat2782
Copy link
Author

ggplot_hist_check.zip

I checked the shape of histograms in the test files with ggplot in R, and they match.

@bbeat2782 bbeat2782 marked this pull request as ready for review July 24, 2023 20:38
Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

can you add some comments in your code? looks like you made some adjustments to the histogram computation, let's ensure it's properly explained what we're doing it (mention we want to mimic what ggplot in R does + explaining each change)

Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

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

I agree with @edublancas.

We might also consider adding some information to our docs about how we calculate the bins and compare it with R to increase trust.

@bbeat2782 bbeat2782 requested a review from neelasha23 as a code owner July 25, 2023 19:07
integration histograms update

revert histogram_categorical

comments
@bbeat2782 bbeat2782 requested review from edublancas and yafimvo July 25, 2023 19:32
@bbeat2782
Copy link
Author

@edublancas @yafimvo
Comments and explanation on our docs (magic-plot.md) are added now.

yafimvo
yafimvo previously approved these changes Jul 26, 2023
Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

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

@bbeat2782 Good job!

@edublancas edublancas merged commit 0233eb0 into ploomber:master Jul 26, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug histograms in ggplot API
3 participants