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

Fixes to the resolve colors function #327

Merged
merged 2 commits into from Mar 10, 2018
Merged

Fixes to the resolve colors function #327

merged 2 commits into from Mar 10, 2018

Conversation

bbengfort
Copy link
Member

This PR provides fixes to the resolve_colors function and adds a test suite to guard against regression. I've also reimplemented resolve_colors on the feature visualizers that had it commented out to use get_color_cycle instead.

This PR unblocks #321 -- however, a more extensive overhaul of color and color handling in YB is required. We have 4 different methodologies that are mixed and matched in kind of a strange fashion. More than that, Yellowbrick-specific requirements have emerged. Briefly, they are:

  • We need discrete colors that are visually separate for classes
  • We need continuous colors for regression values and heatmaps

Both of these requirements are specified by the target variable. Finally, we need helpers for the other utilities we use; but those can be largely handled with resolve_colors and get_color_cycle.

@bbengfort bbengfort added the review PR is open label Mar 8, 2018
@bbengfort bbengfort requested a review from lwgray March 8, 2018 21:38
Copy link
Contributor

@lwgray lwgray left a comment

Choose a reason for hiding this comment

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

You did way more than I expected. You took care of the logic flaw that was preventing the default color scheme from being selected. I see no problems with the rest of the code

cmap = colormap
colormap = cm.get_cmap(colormap)
# Work with the colormap if specified and colors is not
if colormap is not None and colors is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Great, you caught the logic here that was preventing usage of default color scheme.

@bbengfort
Copy link
Member Author

Going to merge this PR even though tests are failing; it looks like something's up with Travis; will open a new branch to try to sort that out.

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.

None yet

2 participants