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 part of #437 #809

Merged
merged 7 commits into from
Apr 12, 2019
Merged

Fixes part of #437 #809

merged 7 commits into from
Apr 12, 2019

Conversation

jimmyshah
Copy link
Contributor

Adds frameon=true from #437 , I closed the previous PR

@jimmyshah
Copy link
Contributor Author

I think I'm having some trouble understanding the problem with my submission. Would it be possible to receive some feedback?

@rebeccabilbro
Copy link
Member

Hey there @jimmyshah and thanks for checking out Yellowbrick and for helping us work through our (ever growing 😅) backlog of issues!

Here's what's happening with your PR; you've globally modified the manual_legend utility, which is used by many of our visualizers. As a result, your change has broken many of those existing visualizers (for instance, ParallelCoordinates, which already passes in a frameon argument).

The request in #437 is to do something similar with Manifold as is currently done in ParallelCoordinates (e.g. passing the frameon kwarg into the call to manual_legend, which already knows how to consume any **legend_kwargs that get passed in), not to modify the manual_legend utility function directly.

Note that when you make this change, it should fix all the CI errors, but we may need to work through some new broken image tests for Manifold — if you want to get a jump on figuring those out, you can read up on our visual image tests here and here!

Apologies for the confusion, and hope this is helpful!

@jimmyshah
Copy link
Contributor Author

Thank you so much for the elaborate response @rebeccabilbro ! It was really helpful and I understand the issue more clearly now. I'm going to try working on this again in a bit.

@jimmyshah
Copy link
Contributor Author

@rebeccabilbro Thank you again for your all of your help! It looks like my commit is passing the Travis CI check according to the site, but it is still showing it as being queued here. Is there something else that I need to do?

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.

Looks great @jimmyshah — all the tests are passing now, so I'll go ahead and merge this in. We hope to see future contributions from you!

One tiny request is that for your next contribution, you use a feature branch (you can read more about our branching conventions here) instead of merging from your develop branch; it makes reviewing easier on us maintainers and ensures your develop stays well-groomed! Thanks again for checking out Yellowbrick and hope to see you around!

@rebeccabilbro rebeccabilbro merged commit b69c608 into DistrictDataLabs:develop Apr 12, 2019
@jimmyshah
Copy link
Contributor Author

@rebeccabilbro This is so wonderful to hear! Thank you so much, and I look forward to future contributions. I can use the feature branch convention going forward as well, my apologies about that.

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