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

Move visualization functions to plotly and get rid of matplotlib dependency #368

Merged
merged 16 commits into from
Nov 8, 2023

Conversation

fealho
Copy link
Member

@fealho fealho commented Oct 13, 2023

Resolve #348.

A notebook testing the new plots can be found here.

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2023

Codecov Report

Attention: 92 lines in your changes are missing coverage. Please review.

Comparison is base (64a91e8) 87.64% compared to head (ddca46c) 86.85%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #368      +/-   ##
==========================================
- Coverage   87.64%   86.85%   -0.79%     
==========================================
  Files          27       27              
  Lines        1772     1788      +16     
==========================================
  Hits         1553     1553              
- Misses        219      235      +16     
Files Coverage Δ
copulas/visualization.py 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fealho fealho marked this pull request as ready for review October 16, 2023 15:05
@fealho fealho requested a review from a team as a code owner October 16, 2023 15:05
@fealho fealho requested review from frances-h and removed request for a team October 16, 2023 15:05
@frances-h
Copy link
Contributor

Can you add some screenshots showing the new plots?

@fealho
Copy link
Member Author

fealho commented Oct 16, 2023

Can you add some screenshots showing the new plots?

@frances-h Added to the description.

@amontanez24
Copy link
Collaborator

setup.py Outdated Show resolved Hide resolved
copulas/visualization.py Show resolved Hide resolved
copulas/visualization.py Show resolved Hide resolved
@frances-h frances-h requested review from frances-h and removed request for frances-h October 24, 2023 14:41
@npatki
Copy link

npatki commented Oct 25, 2023

Can you add some screenshots showing the new plots?

@frances-h Added to the description.

@fealho they are not rendering for me. I think that if you upload a local ipynb notebook into Drive, then Google Colab won't render the graphics. Is it possible to create a notebook directly on Google Colab and run the code from a branch or something? Doing in Colab would be nice bc we can also verify the the interactivity that all plotly graphs come with.

Copy link

@npatki npatki left a comment

Choose a reason for hiding this comment

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

I'm leaving all my feedback here based on the notebook.

Plot Titles

Original issue had mentioned we should update any titles to accurately reflect what is being shown. I didn't see a title for any of the generated plots, we should add that.

I'm basing the proposed titles based off the SDMetrics visualizations, which is what we wanted to align to.

  • dist_1d: Data for column '<name>'. If there is no name (eg. you're passing in a Series without a name, we could leave that out)
  • compare_1d: Real vs. Synthetic Data for column '<name>' (again can leave name out if there is none)
  • scatter_2d: Data for columns '<name>' and '<name>'
  • compare_2d: Real vs. Synthetic Data for columns '<name>' and '<name>'
  • scatter_3d: Data for columns '<name>', '<name>' and '<name>'
  • compare_3d: Real vs. Synthetic Data for columns '<name>', '<name>' and '<name>'

X and Y Axes Labels

For the 1D plots (dist_1d and compare_1d) the axes should be labeled. These can be the same as SDMetrics. (X-axis is value and Y-axis is frequency)

Copy link
Contributor

@frances-h frances-h 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!

@npatki
Copy link

npatki commented Nov 1, 2023

Approved the title and axes changes. Though we should probably update that first notebook as a sanity check.

@fealho fealho merged commit e8bfc9b into main Nov 8, 2023
61 checks passed
@fealho fealho deleted the issue-348-plotly branch November 8, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move visualization functions to plotly and get rid of matplotlib dependency
5 participants