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

[FEATURE] - corr_plot using plotly #116

Merged
merged 3 commits into from
Jul 25, 2023
Merged

[FEATURE] - corr_plot using plotly #116

merged 3 commits into from
Jul 25, 2023

Conversation

m-marqx
Copy link
Contributor

@m-marqx m-marqx commented Jul 23, 2023

name about title labels assignees
Feature Addition interactive version from corr_plot [FEATURE] - corr_plot using plotly Plot @m-marqx

The purpose of this function is to allow users to explore and interact with correlation data dynamically, similar to the corr_plot, but using Plotly instead of Matplotlib.

The function takes the same user parameters as seen in corr_plot(data, split, threshold, target, method, cmap, figsize, annot). The difference between corr_plot and corr_interactive_plot lies in **kwargs to customize the visualization using the heatmap from graph objects from Plotly. I opted for this method and not .imshow from plotly.express because it offers fewer options for user customization.

My original idea was to update the corr_plot with interactive: bool = False and place the function corr_interactive_plot as a private function inside corr_plot. However, this could make the function appear complex and challenging for users to comprehend the interactive version.

klib_plot

@akanz1
Copy link
Owner

akanz1 commented Jul 24, 2023

@m-marqx Thank you for your contribution, this looks great! I will have a close look in the next 1-2 days and get back to you. :)

Copy link
Owner

@akanz1 akanz1 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 except for the minor type hint updates. Once those have been addressed we cna merge this and i'll play around with it a bit before drafting a new release. Thanks again!

src/klib/describe.py Outdated Show resolved Hide resolved
src/klib/describe.py Outdated Show resolved Hide resolved
src/klib/describe.py Outdated Show resolved Hide resolved
@akanz1
Copy link
Owner

akanz1 commented Jul 24, 2023

Also, please merge in main as there have been some minor changes in other parts of the code (shouldn't give you any conflicts).

Co-Authored-By: Andreas Kanz <51492342+akanz1@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented Jul 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@m-marqx
Copy link
Contributor Author

m-marqx commented Jul 24, 2023

I'm glad you liked the contribution!

I hope the changes will be helpful for the project.

Let me know if there's anything else I can assist with.

@akanz1
Copy link
Owner

akanz1 commented Jul 25, 2023

Linting failed but i will deal with this once i do some test runs.

If you are interested you can add an image of the new plot to the documentation (readme.md and in /examples) similar to what is already there, probably best to add it below the corr_plot image.

@akanz1 akanz1 merged commit 3d130d3 into akanz1:main Jul 25, 2023
4 of 16 checks passed
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.

2 participants