Skip to content

Added %sqlplot tests #628

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

Merged
merged 9 commits into from
Jun 21, 2023
Merged

Added %sqlplot tests #628

merged 9 commits into from
Jun 21, 2023

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