Skip to content

Conversation

@mehtamohit013
Copy link

@mehtamohit013 mehtamohit013 commented Jun 20, 2023

Describe your changes

Added %sqlplot tests for boxplot and histogram

Issue number

Closes #526

Checklist before requesting a review


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

@mehtamohit013 mehtamohit013 added the no-changelog This PR doesn't require a changelog entry label Jun 20, 2023
@mehtamohit013 mehtamohit013 marked this pull request as ready for review June 20, 2023 12:57
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.

looks good.

@mehtamohit013
Can we add more complex cases (a large number of bins (100/250/500), 2 plots, custom style, vertical/horizontal bar)? For this, we'll probably need a more complete dataset.

We can use these as references:

plot

ggplot

@mehtamohit013
Copy link
Author

@yafimvo

  • Changed the dataset.
  • Modified the bin size to 300
  • Added custom graph (i.e. added grid)
  • Histogram doesn't support orientation, so not added

What do you mean by 2 plots? 2 histogram in one?

TLDR: Added all the plots present in %sqlplot doc

@mehtamohit013 mehtamohit013 requested a review from yafimvo June 21, 2023 11:29
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.

@mehtamohit013
lgtm. Just one comment about the fixture and I think it's ready for merge.

What do you mean by 2 plots? 2 histogram in one?

Yes. I see you already did it.

@mehtamohit013 mehtamohit013 requested a review from yafimvo June 21, 2023 13:12
@edublancas edublancas merged commit e5c341e into ploomber:master Jun 21, 2023
@mehtamohit013 mehtamohit013 deleted the tests branch August 8, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog This PR doesn't require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

missing image tests for %sqlplot

3 participants