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

Addresses Issue #615 : Adding new parameters "Colorbar" and "Heatmap" in PCA visualizer #812

Closed
wants to merge 4 commits into from

Conversation

percygautam
Copy link
Contributor

The following changes were made to pca.py referenced in #615 :

  • Added new optional parameters 'Colorbar' and 'Heatmap' which shows the magnitude of each feature value to the component, for both '2D' and '3D' cases.

Next Step:

  • Updating Documentation with a proper explanation on how to use these two optional parameters when using PCA visualizer.

P.S: I am adding a snippet showing PCA visualization of the dataset credit from yellowbrick.datasets, when both the parameters are True and proj_dim = 2.

PCA

@rebeccabilbro
Copy link
Member

Hey there @percygautam and thanks for checking out Yellowbrick! We recently merged in a PR that also touched yellowbrick/features/pca.py, so before we proceed with the review, we'll need to resolve the conflicts in that file. Are you up for taking a crack at pulling from develop, resolving any conflicts on your machine, and pushing the updates to this same branch? Let us know if you have any questions!

@rebeccabilbro rebeccabilbro self-assigned this Apr 15, 2019
@percygautam
Copy link
Contributor Author

@rebeccabilbro I am working on resolving the conflicts due to the recently merged PR.

@percygautam
Copy link
Contributor Author

@rebeccabilbro I have resolved the conflict raised by the recent PR. Heatmap and Colorbar, as well as alpha parameter, are working fine in my machine

Copy link
Member

@rebeccabilbro rebeccabilbro left a comment

Choose a reason for hiding this comment

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

Hi there @percygautam and apologies for the slow response! Thanks so much for resolving that prior merge conflict; we have had a flurry of contributions lately and many of them are touching the same files, so I really appreciate your help in keeping your branch in sync with develop.

I took a quick initial look at your code and wanted to provide some preliminary high-level feedback. It looks like you are definitely on the right track, and the sample plot looks terrific. However, I'm noticing some API issues that we'll need to work through before we go further.

In both your transform and finalize methods, you are modifying the plt directly. Unfortunately the Yellowbrick API requires that visualizers use the ax object passed to them and not plt. Generally, visualizers should behave as though they are a plot that is part of a larger figure (you can read more about this here).

The rationale for this is that it ensures that users can call the visualizer alone or in the context of a figure that contains multiple visualizers (e.g. a "visual pipeline" see #498). If the user does not pass a specific ax then the global current axes will be used via plt.gca (note that this is already part of the abstract base class, so will be done automatically when you subclass MultiFeatureVisualizer).

My recommendation would be to use something akin to what we do in the JointPlotVisualizer, where we have a private method that divides the axes up if certain parameters have been set (e.g. colorbar and heatmap in your case). I realize that this adds a fair amount of complexity to the task, so let us know if we can help at all! Thanks again for taking on this challenging and important task!

@percygautam
Copy link
Contributor Author

Kon'nichiwa! @rebeccabilbro
I understand the problem and the importance of having a separate private method for dividing the axis.
I'll get right into solving this problem.
I'll keep in touch about the progress.

@rebeccabilbro rebeccabilbro added the gone-stale PR or issue has not seen activity in >30 days and/or develop branch has since diverged significantly label May 10, 2019
@rebeccabilbro
Copy link
Member

Hey there @percygautam! Is this still something you're planning to work on?

@percygautam
Copy link
Contributor Author

Hey there, I didn't get the time to work because of my semester examinations. Now that they are over, I'll continue to work on the task at hand. Will try to make a submission by next weekend.

@rebeccabilbro
Copy link
Member

Ok great, looking forward to it!

@rebeccabilbro
Copy link
Member

Hi again @percygautam — since it looks like the conversation here has gone a bit stale, I'm going to go ahead and archive this PR for now. If you end up having time to come back to this, feel free to re-open this PR or open a new one, and thanks again for checking out Yellowbrick!

@percygautam
Copy link
Contributor Author

@rebeccabilbro I have updated the files as you have advised. Should I re-open the PR or make a new one?

@rebeccabilbro
Copy link
Member

Hey there @percygautam! So glad to have you working on this! Since the current edits touch only one file, it should be safe to reopen this PR (though we'd pretty quickly need to do a pull from develop to make sure you have all the most recent changes, which can sometimes get a little tangly). Alternatively, if your develop branch is synced with ours, it might be easiest to open a new feature branch off of that and cut a new PR with your changes to pca.py. Either way, look forward to seeing this contribution!

@percygautam percygautam deleted the PCA_feature branch June 12, 2019 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gone-stale PR or issue has not seen activity in >30 days and/or develop branch has since diverged significantly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants