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

Updated Manifold to extend Projection Visualizer #930

Merged
merged 17 commits into from
Jul 27, 2019

Conversation

naresh-bachwani
Copy link
Contributor

@naresh-bachwani naresh-bachwani commented Jul 22, 2019

This PR updates Manifold to extend Projection Visualizer as a part of issue #874.

Following changes are made to Manifold:

  1. It now handles 3d plot.
  2. Removed duplicate code.
  3. Added new parameters like projection

Sample plot

from yellowbrick.datasets import load_credit
from yellowbrick.features import Manifold

oz = Manifold(manifold='tsne')
oz.fit_transform(*load_credit())
oz.poof(outpath="tsne_credit.png")

tsne_credit

TODOs and questions

Still to do:

  • Add tests.

  • Is the commit message formatted correctly?

  • Have you noted the new functionality/bugfix in the release notes of the next release?

  • Included a sample plot to visually illustrate your changes?

  • Do all of your functions and methods have docstrings?

  • Have you added/updated unit tests where appropriate?

  • Have you updated the baseline images if necessary?

  • Have you run the unit tests using pytest?

  • Is your code style correct (are you using PEP8, pyflakes)?

  • Have you documented your new feature/functionality in the docs?

  • Have you built the docs using make html?

@bbengfort bbengfort self-requested a review July 25, 2019 16:40
@bbengfort bbengfort changed the title [WIP] Updated Manifold to extend Projection Visualizer Updated Manifold to extend Projection Visualizer Jul 25, 2019
Copy link
Member

@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

Glorious code simplification! All of your hard work in the ProjectionVisualizer has paid off and now Manifold makes use of all that functionality and is so much more than it was before! It's awesome to see a plan come together in this way! Very good work!

I have a few minor requests mostly related to documentation, but then we should be good to go.

One question I have looking through the new baseline images - it appears that test_manifold_classification.png and test_manifold_classification_3d.png have sequential instead of discrete colors. I think this is because of the default of setting the colormap in DataVisualizer ... we may have to remove that because of how it changes Manifold.

Also, I want to talk about style and communication in this plot. Would you mind running the following code and posting the resulting image in the PR?

from yellowbrick.datasets import load_credit

oz = Manifold(manifold='tsne')
oz.fit_transform(*load_credit())
oz.poof(outpath="tsne_credit.png")

tests/test_features/test_manifold.py Show resolved Hide resolved
tests/test_features/test_manifold.py Show resolved Hide resolved
yellowbrick/features/manifold.py Show resolved Hide resolved
yellowbrick/features/manifold.py Show resolved Hide resolved
yellowbrick/features/manifold.py Show resolved Hide resolved
yellowbrick/features/manifold.py Outdated Show resolved Hide resolved
yellowbrick/features/manifold.py Show resolved Hide resolved
yellowbrick/features/manifold.py Show resolved Hide resolved
yellowbrick/features/manifold.py Show resolved Hide resolved
yellowbrick/features/manifold.py Show resolved Hide resolved
Copy link
Member

@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes!

else:
# Technically this should never be raised
raise NotFitted("could not determine target color type")
self.draw(Xp, y, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I couldn't see this thanks to the big diff between things!

Copy link
Member

@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

@naresh-bachwani looks great, thank you for making those changes!

@bbengfort bbengfort merged commit c6ee8ee into DistrictDataLabs:develop Jul 27, 2019
@naresh-bachwani naresh-bachwani deleted the manifold-viz branch July 27, 2019 20:14
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.

3 participants