Skip to content

hide tick labels on boxplots in scatterplotmatrix #623

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

Merged
merged 2 commits into from
Dec 2, 2016

Conversation

Kully
Copy link
Contributor

@Kully Kully commented Dec 1, 2016

No description provided.

@Kully
Copy link
Contributor Author

Kully commented Dec 1, 2016

-provided diag options in scatterplotmatrix doc string
-made function to hide tick labels in subplots that use box plots
-updated test in test_figurefactory to look at a chart output with diag='box'

@theengineear Let me know if this looks good to you. 🐔 No rush.

@Kully Kully changed the title updated test with diag=box to test new change hide tick labels on boxplots in scatterplotmatrix Dec 1, 2016
Copy link
Contributor

@theengineear theengineear left a comment

Choose a reason for hiding this comment

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

Non-blocking comments, but a little cleanup, documentation would be nice :) Also, is there an open issue that this closes?

# 'domain': [0.0, 0.425], 'title': 'Fruit'},
# 'yaxis4': {'anchor': 'x4',
# 'domain': [0.0, 0.425]}}
# }
Copy link
Contributor

Choose a reason for hiding this comment

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

⚡️ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoopsi, forgot to erase

@@ -3881,6 +3881,9 @@ def _scatterplot(dataframe, headers, diag, size,
title=title,
showlegend=True
)

fig = FigureFactory._hide_tick_labels_from_box_subplots(fig)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this to something more generic now? I.e., the box in there? Just looks funny to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm. this is new! i'll comment down there :)

@@ -4559,6 +4563,23 @@ def _scatterplot_theme(dataframe, headers, diag, size, height,
return fig

@staticmethod
def _hide_tick_labels_from_box_subplots(fig):
"""
Hides tick labels for box plots in scatterplotmatrix subplots.
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

for j, plot_data in enumerate(fig['data']):
if plot_data['type'] == 'box':
boxplot_xaxes.append(
'xaxis{}'.format(plot_data['xaxis'][-1])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% convinced this is correct. Doesn't x1 map to xaxis in layout? ORRRRR, is it because in plotly.py we don't use xaxis, instead using xaxis1? Mind throwing a code comment in here?

Hides tick labels for box plots in scatterplotmatrix subplots.
"""
boxplot_xaxes = []
for j, plot_data in enumerate(fig['data']):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the j and enumerate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we usually call plot_data --> trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno what other people do, but my 👍 indicate that I agree with your comment and have already made that change to my local files (before pushing)

Do you recommend something else or is this all good? Just for clarity and global consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

The 👍 seems to be popular/common.

for xaxis in boxplot_xaxes:
fig['layout'][xaxis]['showticklabels'] = False

return fig
Copy link
Contributor

Choose a reason for hiding this comment

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

Personal preference, but I'm not a huge fan of the chaining pattern. You're mutating fig in this function and it makes it super clear that that's your intention if you don't return anything.

I.e., don't return fig to make it clear to callers that this function mutates the fig you give it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes a lot of sense. I figured that you'd need to assign back to fig cause was pointing to another fig and not mutating, but I suppose that's what it's doing.

@@ -4806,7 +4827,8 @@ def create_scatterplotmatrix(df, index=None, endpts=None, diag='scatter',
that defines intervals on the real line. They are used to group
the entries in an index of numbers into their corresponding
interval and therefore can be treated as categorical data
:param (str) diag: sets the chart type for the main diagonal plots
:param (str) diag: sets the chart type for the main diagonal plots.
The options are 'scatter', 'histogram' and 'box'.
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@Kully
Copy link
Contributor Author

Kully commented Dec 2, 2016

Non-blocking comments, but a little cleanup, documentation would be nice :) Also, is there an open issue that this closes?

Don't think so, but a user complained about it. Plus, I had been trying to figure this out a while ago. Is it better to have an issue to link to to make us look productive? 😛

@theengineear
Copy link
Contributor

Is it better to have an issue to link to to make us look productive?

Well, it's sorta nice to have a sense of a queue that we're getting work done in. No huge deal though.

@Kully Kully merged commit 58db737 into master Dec 2, 2016
@Kully Kully deleted the remove-scatterplotmatrix-labels branch December 2, 2016 17:06
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.

2 participants