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

BUG: Fixes color selection in andrews_curve #5378

Merged
merged 1 commit into from Oct 31, 2013

Conversation

Projects
None yet
3 participants
@tacaswell
Contributor

tacaswell commented Oct 30, 2013

Fixed bug introduced in b5265a5 which change from picking a single random color for the class to picking a random color for each line.

This returns the color selection to a per-set basis.

See http://stackoverflow.com/questions/19667209/colors-in-andrews-curves

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

Please post an image of output you get from this.

Contributor

jtratner commented Oct 30, 2013

Please post an image of output you get from this.

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

(and nice to have reproducible example, even if it's just copying from SO question)

Contributor

jtratner commented Oct 30, 2013

(and nice to have reproducible example, even if it's just copying from SO question)

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Oct 30, 2013

Contributor
import pandas as pd
import numpy as np

df = pd.DataFrame(np.random.randn(100,3), columns=['A','B', 'C'])
df['X'] = np.random.choice(['Alpha', 'Beta', 'Theta'], size=100)
pd.tools.plotting.andrews_curves(df, 'X')

so

Contributor

tacaswell commented Oct 30, 2013

import pandas as pd
import numpy as np

df = pd.DataFrame(np.random.randn(100,3), columns=['A','B', 'C'])
df['X'] = np.random.choice(['Alpha', 'Beta', 'Theta'], size=100)
pd.tools.plotting.andrews_curves(df, 'X')

so

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

thanks for posting that - this looks good to me (going to try to pull down and see if I get the same thing) - anyone have any comments?

Contributor

jtratner commented Oct 30, 2013

thanks for posting that - this looks good to me (going to try to pull down and see if I get the same thing) - anyone have any comments?

@ghost ghost assigned jtratner Oct 30, 2013

@amanahuja

This comment has been minimized.

Show comment
Hide comment
@amanahuja

amanahuja Oct 30, 2013

Looks great now; thanks!

Let me know if this needs to be a separate request, but consider:

plt.ylabel(r'$f_x(t)$')
plt.xlabel('t')
ax.xaxis.set_ticks([-pi, -pi/2, 0, pi/2, pi])
ax.xaxis.set_ticklabels([r'$-\pi$', r'$-\frac{\pi}{2}$', '0', r'$\frac{\pi}{2}$', r'$\pi$', ])

Everyone loves properly labeled plots.

so_andrewscurves_03

amanahuja commented Oct 30, 2013

Looks great now; thanks!

Let me know if this needs to be a separate request, but consider:

plt.ylabel(r'$f_x(t)$')
plt.xlabel('t')
ax.xaxis.set_ticks([-pi, -pi/2, 0, pi/2, pi])
ax.xaxis.set_ticklabels([r'$-\pi$', r'$-\frac{\pi}{2}$', '0', r'$\frac{\pi}{2}$', r'$\pi$', ])

Everyone loves properly labeled plots.

so_andrewscurves_03

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

Can you post another image? :)

Contributor

jtratner commented Oct 30, 2013

Can you post another image? :)

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

Let's keep this to the colors for now and can deal with labeling
separately.

Contributor

jtratner commented Oct 30, 2013

Let's keep this to the colors for now and can deal with labeling
separately.

@amanahuja

This comment has been minimized.

Show comment
Hide comment
@amanahuja

amanahuja Oct 30, 2013

Just updated my last comment with an image. :) I think having the grid lines in multiples of pi makes a world of a difference.

amanahuja commented Oct 30, 2013

Just updated my last comment with an image. :) I think having the grid lines in multiples of pi makes a world of a difference.

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

The colors change is minimal enough that I'm willing to merge it (and it seems clearly correct). We're in the middle of putting out a release so, in terms of changing labels or ticks, I'd prefer to hold off on that for a week or two. You can put up a separate PR that we can consider though.

Contributor

jtratner commented Oct 30, 2013

The colors change is minimal enough that I'm willing to merge it (and it seems clearly correct). We're in the middle of putting out a release so, in terms of changing labels or ticks, I'd prefer to hold off on that for a week or two. You can put up a separate PR that we can consider though.

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

It looks like you have Travis set up but that it didn't build this. If you think Travis is set up can you run git commit --amend -C HEAD && git push --force to get Travis to notice the build?

Contributor

jtratner commented Oct 30, 2013

It looks like you have Travis set up but that it didn't build this. If you think Travis is set up can you run git commit --amend -C HEAD && git push --force to get Travis to notice the build?

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Oct 30, 2013

Contributor

I hadn't enabled it to build. Travis is running now.

Contributor

tacaswell commented Oct 30, 2013

I hadn't enabled it to build. Travis is running now.

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Oct 30, 2013

Contributor

And it looks like it built my master branch (which is just master).

Contributor

tacaswell commented Oct 30, 2013

And it looks like it built my master branch (which is just master).

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

Need to check out the fix_andrews... branch

Contributor

jtratner commented Oct 30, 2013

Need to check out the fix_andrews... branch

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Oct 30, 2013

Contributor

Sorry, I am really confused by your travis set up (I am used to mpl where PRs are auto-magically built by the matplotlib travis account).

Contributor

tacaswell commented Oct 30, 2013

Sorry, I am really confused by your travis set up (I am used to mpl where PRs are auto-magically built by the matplotlib travis account).

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 30, 2013

Contributor

you did it :)

If there's a way to do that in Travis, we probably should - that would be much easier for contributors.

Contributor

jtratner commented Oct 30, 2013

you did it :)

If there's a way to do that in Travis, we probably should - that would be much easier for contributors.

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 31, 2013

Contributor

Can you add a quick note to release.rst? After that I'll merge.

Contributor

jtratner commented Oct 31, 2013

Can you add a quick note to release.rst? After that I'll merge.

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Oct 31, 2013

Contributor

Done.

Squashed the commits.

Contributor

tacaswell commented Oct 31, 2013

Done.

Squashed the commits.

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 31, 2013

Contributor

If you could, could you change the commit name to something like: "BUG: Fix color selection in andrews curve" and then you can put "fixed bug introduced in b5265a5
which change from picking a single random color for the class to
picking a random color for each line."

In the commit message? Makes it easier to follow what's going on.

Contributor

jtratner commented Oct 31, 2013

If you could, could you change the commit name to something like: "BUG: Fix color selection in andrews curve" and then you can put "fixed bug introduced in b5265a5
which change from picking a single random color for the class to
picking a random color for each line."

In the commit message? Makes it easier to follow what's going on.

BUG: Fix color selection in andrews curve"
fixed bug introduced in b5265a5
which change from picking a single random color for the class to
picking a random color for each line.
@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Oct 31, 2013

Contributor

like that?

Contributor

tacaswell commented Oct 31, 2013

like that?

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 31, 2013

Contributor

exactly. once it passes Travis I'll merge - thanks for the final cleanups!

Contributor

jtratner commented Oct 31, 2013

exactly. once it passes Travis I'll merge - thanks for the final cleanups!

jtratner added a commit that referenced this pull request Oct 31, 2013

Merge pull request #5378 from tacaswell/fix_andrews_colors
BUG: Fixes color selection in andrews_curve

@jtratner jtratner merged commit f5b3f8a into pandas-dev:master Oct 31, 2013

@jtratner

This comment has been minimized.

Show comment
Hide comment
@jtratner

jtratner Oct 31, 2013

Contributor

thanks!

Contributor

jtratner commented Oct 31, 2013

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment