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

initial build-out of facet wrapping #1838

Merged
merged 5 commits into from
Oct 22, 2019
Merged

initial build-out of facet wrapping #1838

merged 5 commits into from
Oct 22, 2019

Conversation

nicolaskruchten
Copy link
Contributor

@nicolaskruchten
Copy link
Contributor Author

The Percy diffs are because this code generates x domains that range from 0-1 instead of master which goes from 0-0.98. 0-1 seems "more correct" to me... @jonmmease any insight into why the previous version does what it does?

@nicolaskruchten
Copy link
Contributor Author

Ah I found it: https://github.com/plotly/plotly.py/blob/master/packages/python/plotly/plotly/subplots.py#L529

Basically I'm now passing a length-0 element to row_titles in single-facet cases, not [None] and so it skips this line.

OK so I'm fine with these Percy diffs. Ready for review!

I guess I need some tests.

@emmanuelle
Copy link
Contributor

It was not introduced by this PR, but the behaviour when there is a very large number of cols/rows is weird. For example

px.scatter(gapminder, x="pop", y="lifeExp", facet_row="country")

or

px.scatter(gapminder, x="pop", y="lifeExp", facet_col="country", facet_col_wrap=8)

give errors like

~/code/plotly.py/packages/python/plotly/_plotly_utils/basevalidators.py in raise_invalid_val(self, v, inds)
    281                 typ=type_str(v),
    282                 v=repr(v),
--> 283                 valid_clr_desc=self.description(),
    284             )
    285         )

ValueError: 
    Invalid value of type 'builtins.float' received for the 'domain[1]' property of layout.yaxis
        Received value: -0.010555555555555565

    The 'domain[1]' property is a number and may be specified as:
      - An int or float in the interval [0, 1]

which is not very helpful as an error message. Should we either introduce a limit in the number of cols/rows, or try to catch this and output a better message?

@emmanuelle
Copy link
Contributor

Thank you for fixing the bug of #1836.

@@ -147,6 +147,11 @@
colref,
"Values from this column or array_like are used to assign marks to facetted subplots in the horizontal direction.",
],
facet_col_wrap=[
"int",
"Wrap the column variable at this width, so that the column facets span multiple rows.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say "number of columns. Facets are wrapped at this number to span multiple rows". Maybe it's just me, but since I'm not familiar with this kind of representation I had a hard time understanding what this was before I actually tried 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.

Done!

@emmanuelle
Copy link
Contributor

ok, I just left a doc suggestion which you might or might not implement. My comment about the large number of cols/rows is not specific to this PR and can go to an issue. 💃

@nicolaskruchten
Copy link
Contributor Author

Nice catch on the "many columns" thing, here's the issue I broke it off into: #1839

@nicolaskruchten
Copy link
Contributor Author

OK I've added percy tests for the two tricky cases of facet wrapping, and a test locking down the fix for #1836

@nicolaskruchten
Copy link
Contributor Author

The plotlyjs_dev_build test failure looks like something which will go away the next time there's a master commit on plotly.js

@nicolaskruchten nicolaskruchten merged commit 049d837 into master Oct 22, 2019
@nicolaskruchten nicolaskruchten mentioned this pull request Nov 12, 2019
@nicolaskruchten nicolaskruchten deleted the facet_wrap branch June 19, 2020 16:13
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.

Passing data frame column to facet_col raises exception Facet wrapping
2 participants