Carson scales free #167

cpsievert · 2015-02-18T00:22:06Z

The purpose of this pull request is to fully implement facet_wrap(..., scales = 'free').

It also seems possible to implement space = "free", but perhaps that should be in a separate pull request.

cpsievert · 2015-02-18T00:28:19Z

You might find this script I wrote helpful for comparing the output of gg2list() between two different package versions. When I Rscript json-diffs.R, I can see colorized diffs in my console like this:

Installing from GitHub is perhaps inefficient, but it might be worth avoiding nasty git checkout errors. Let me know if any ideas on how to improve it!

cpsievert · 2015-02-18T00:33:16Z

Ah, crud. I messed with the white space a bit which makes the last commit diff hard to read. You can add ?w=1 to the end of url to ignore whitespace, for example, this is better to look at

mkcor · 2015-02-18T01:05:22Z

@cpsievert Regarding whitespaces, I go for RStudio's indentation. In this respect, it looks like some of your whitespace changes are 'fixes', so that's all good. I'm not sure about others (see inline comments).

mkcor · 2015-02-18T01:07:05Z

R/ggplotly.R

+shape=function(pch) {
+  pch2symbol[as.character(pch)]
+},
+direction=identity)


Why this change? Please keep it the way RStudio does indentation.

Hmm, no good reason. I'll change that back

cpsievert · 2015-02-18T01:09:48Z

Sweet. That's what I prefer to.

I also prefer

f <- function() {
  print("hi")
}

over

f <- function() 
  {
    print("hi")
  }

so I'd be happy to change that as well

chriddyp · 2015-02-18T01:10:46Z

Nice! hey @tdhock is it possible/easy to run that test table on this branch?

cpsievert · 2015-02-18T01:22:51Z

I'm pretty sure Toby's current implementation would only support visual testing of one branch at a time. Perhaps we could do something similar in a "username.github.com" repo with a folder for each branch?

tdhock · 2015-02-18T19:35:53Z

yes it should be possible to run the test table on this branch, but I will need to do a little hacking. Will post a link to the updated table when I get a chance to work on it.

tdhock · 2015-02-18T21:53:27Z

check out the test table

http://ropensci.github.io/plotly-test-table/tables/314a6ec37d1d423cc90120903b0a1deb0b4eb717/index.html

there are some cases with weird disappearing boxes

http://ropensci.github.io/plotly-test-table/tables/314a6ec37d1d423cc90120903b0a1deb0b4eb717/ticks-boxes-scales-free.html

(not your fault, this test only exists on the toby-fixes branch)

https://github.com/ropensci/plotly/blob/toby-fixes/tests/testthat/test-ggplot-ticks.R#L51

cpsievert · 2015-02-18T22:07:21Z

Sweet, thanks @tdhock! Will toby-fixes be merged into master soonish? Or maybe we should merge carson-scales-free and toby-fixes?

chriddyp · 2015-02-18T22:29:11Z

Cool - @tdhock @mkcor - should we move these *.r files that have the cookbook examples branch (https://github.com/ropensci/plotly/tree/add-r-cookbook-tests/tests/cookbook-test-suite) into the testhat directory (or elsewhere) in master so that the test-table can pick up on that rich set of examples?
@tdhock I'm assuming that checkout out the add-r-cookbook branch and running those additional tests would add too much complexity to your table generating script (https://github.com/ropensci/plotly-test-table/blob/gh-pages/index.R)? @mkcor if your opposed to having these non-test scripts inside that folder, then maybe we can move them somewhere else, like a plotly/scripts folder or @tdhock maybe we can move them into the plotly-test-table repo to be included?
i'd love to just visually inspect all of the examples in the facets chapter (https://github.com/ropensci/plotly/blob/add-r-cookbook-tests/tests/cookbook-test-suite/facets.r) before merging this PR

tdhock · 2015-02-19T14:39:46Z

@cpsievert I don't know how long it will be before toby-fixes it merged into master, so I would suggest to pull changes from toby-fixes into your branch.

@chriddyp @mkcor I agree that it is a good idea to add examples from the cookbook branch to the test table. In my opinion the easiest way to do this would be to just add them as tests/testthat/test-*.R files under toby-fixes. I can do that if you guys tell me which of

https://github.com/ropensci/plotly/tree/add-r-cookbook-tests/tests/cookbook-test-suite

you would like to include... or would you like to include all of the *.r and *.R files?

chriddyp · 2015-02-19T14:46:24Z

@tdhock OK, that sounds good. All of them are good except multiple_graphs_on_one_page.r

mkcor · 2015-02-19T22:39:27Z

@cpsievert [On curly braces] Absolutely. It's not only a preference, it's our style guide (https://github.com/ropensci/plotly/wiki/Development-guidelines#coding-style, https://google-styleguide.googlecode.com/svn/trunk/Rguide.xml#curlybraces).

mkcor · 2015-02-19T23:10:30Z

@tdhock @chriddyp Definitely, let's include the cookbook examples as regular .R files (not .r) under tests/testthat/. But please create a new branch for this and keep each PR as small as possible!!

mkcor · 2015-02-19T23:13:55Z

@cpsievert @tdhock toby-fixes could be merged sooner if it was smaller. The bigger the PR, the longer it takes to review. It should not be toby-fixes in the first place: There should be toby-errorbar-horizontal, toby-box-scales-free, etc. Each branch and corresponding PR should address one issue.

mkcor · 2015-02-19T23:16:56Z

@cpsievert @tdhock For reference, https://github.com/ropensci/plotly/wiki/Development-guidelines#a-few-guidelines -- Thanks @chriddyp for updating and cleaning that up!

mkcor · 2015-02-19T23:22:19Z

@cpsievert Can you please revert c542520 ? You shouldn't find yourself merging another feature/dev/wip branch into your own feature branch. Merging master makes sense if it has changed meanwhile (basically, you would be merging toby-fixes indirectly, because it would have been merged into master, but this is not the case).
Once you revert this merge, I can do a final review.

cpsievert · 2015-02-19T23:37:34Z

@mkcor ok, I removed that merge from the history on this branch

mkcor · 2015-02-20T16:57:06Z

@cpsievert Amazing! Just pulled your branch for a quick final review.

mkcor · 2015-02-20T17:00:17Z

R/ggplotly.R

@@ -168,6 +170,9 @@ gg2list <- function(p){
    gglayout <- built$panel$layout
    ## invert rows so that plotly and ggplot2 show panels in the same order
    gglayout$plotly.row <- max(gglayout$ROW) - gglayout$ROW + 1
+    ## ugh, ggplot counts panel right-to-left & top-to-bottom
+    ## plotly count them right-to-left & *bottom-to-top*


Please use a single # for comments (see https://github.com/ropensci/plotly/wiki/Development-guidelines#coding-style and https://google-styleguide.googlecode.com/svn/trunk/Rguide.xml#comments).

tdhock · 2015-02-25T23:04:48Z

just re-made the test table on your branch

http://ropensci.github.io/plotly-test-table/tables/e1b76fffdaec64085aa2665a3b469a943014b8ee/index.html

I noticed a couple of regressions, can you please check them out?

http://ropensci.github.io/plotly-test-table/tables/e1b76fffdaec64085aa2665a3b469a943014b8ee/ticks-boxes-scales-free.html

http://ropensci.github.io/plotly-test-table/tables/e1b76fffdaec64085aa2665a3b469a943014b8ee/facet_wrap_free_y.html

http://ropensci.github.io/plotly-test-table/tables/e1b76fffdaec64085aa2665a3b469a943014b8ee/facet_wrap_free_x.html

http://ropensci.github.io/plotly-test-table/tables/e1b76fffdaec64085aa2665a3b469a943014b8ee/facet_grid_free.html

tdhock · 2015-02-25T23:06:33Z

by the way do you want to get added to ropensci so you can add a branch under the main plotly repos for your next branch/PR? i guess it doesn't really matter but I figured I would ask.

cpsievert · 2015-02-25T23:28:17Z

Thanks @tdhock. I'll hopefully have a fix soon. It'd be nice to be added, but not essential.

chriddyp · 2015-03-09T20:00:30Z

Hey @cpsievert - how is this going? anything I can do to help get this through? A few users wrote in about scales-free this weekend!

cpsievert · 2015-03-09T20:11:30Z

I just updated the test table (note that it didn't update automatically because this PR was initiated from my fork).

http://ropensci.github.io/plotly-test-table/

I don't see any problems, and I'd be OK with merging now, but we can certainly add more of the cookbook tests if you'd like.

chriddyp · 2015-03-09T20:17:39Z

Fantastic, the visual tests look good! 👍 from me

cpsievert · 2015-03-09T20:20:44Z

Cool, thanks. Should I wait for a +1 from @mkcor?

chriddyp · 2015-03-09T20:22:14Z

@mkcor is on vacation till the end of the week, I think she'll be reviewing post-merge post-vacation. @tdhock could you take a closer look at the code for approval?

cpsievert · 2015-03-09T20:23:51Z

OK, I actually just noticed a regression in facet strip labels. I'll fix later today and let everyone know when it's ready.

cpsievert · 2015-03-09T22:19:23Z

Annotations are fixed now. Ready to merge when you are @chriddyp @tdhock

cpsievert · 2015-03-09T22:32:05Z

@tdhock it appears your latest push to plotly-test-table somehow overwrote homepage links to commits I made earlier today --

http://ropensci.github.io/plotly-test-table/

You can still see them here though --

http://ropensci.github.io/plotly-test-table/tables/f800c1249fc36072c1b4dd775b88659144b80f17/

tdhock · 2015-03-09T22:35:49Z

The homepage links are generated via the files in tables/* so I think the links will show up the next time the table is re-made.

cpsievert · 2015-03-10T18:10:33Z

@tdhock @chriddyp please have a look at the table and +1 (it looks good to me and I'm ready to merge)

http://ropensci.github.io/plotly-test-table/tables/f9e252af61276fd40168fc8916024877c7f2f6a3/

There was a failure in downloading contours (this seems to be quite common when I make locally):

http://ropensci.github.io/plotly-test-table/tables/f9e252af61276fd40168fc8916024877c7f2f6a3/contour.html

However, this test for the previous commit looks good (the only is difference is NEWS/DESCRIPTION)

http://ropensci.github.io/plotly-test-table/tables/f800c1249fc36072c1b4dd775b88659144b80f17/contour.html

tdhock · 2015-03-10T19:10:06Z

are you sure it is a download error? If so there should be a log file... if there is not a log file then there is a bug in the plotly-test-table code.

cpsievert · 2015-03-10T19:19:30Z

I don't see a log -- here is what I see in my terminal

tdhock · 2015-03-10T19:42:38Z

If it is a download error you get a log line via this code https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L248-L250

but you are seeing an error due to this code https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L265-L270

the only reason why I can think that would be happening is that the py$ggplotly function returns an error
https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L233

that is the code for SENDING the plotly, not for downloading it. I guess I may need to add some code to attempt retries for the sending step as well.

cpsievert · 2015-03-10T19:49:49Z

OK, good to know, it's probably because I'm on a crappy wifi connection
right now. I don't think it should prevent this from being merged, agreed?

On Tue, Mar 10, 2015 at 2:42 PM, Toby Dylan Hocking <
notifications@github.com> wrote:

If it is a download error you get a log line via this code
https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L248-L250

but you are seeing an error due to this code
https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L265-L270

the only reason why I can think that would be happening is that the
py$ggplotly function returns an error
https://github.com/ropensci/plotly-test-table/blob/gh-pages/table.R#L233

that is the code for SENDING the plotly, not for downloading it. I guess I
may need to add some code to attempt retries for the sending step as well.

—
Reply to this email directly or view it on GitHub
#167 (comment).

cpsievert · 2015-03-10T20:55:56Z

I made the table again (after merging with master which now has the fix for ribbon transparency) and everything looks good (with no caveats :)

http://ropensci.github.io/plotly-test-table/tables/bfa5bfef02ce059712ecb08d38106ddace3cd12e/index.html

Feel free to merge @chriddyp @tdhock 👍

tdhock · 2015-03-10T21:00:24Z

+1

Carson scales free

cpsievert added 3 commits February 16, 2015 12:05

Ignore RStudio projects

481abd9

update docs

c1742df

Implement free scales for facet_wrap. Fixes #166

12f471a

mkcor reviewed Feb 18, 2015
View reviewed changes

Fix identation of aesConverters

19e42ab

mkcor reviewed Feb 20, 2015
View reviewed changes

cpsievert added 3 commits February 26, 2015 14:20

merge conflicts with master

0108095

fix attempt

05d29e7

merge with master (again)

f5684fd

chriddyp mentioned this pull request Mar 9, 2015

Toby cookbook-axes #172

Merged

chriddyp mentioned this pull request Mar 9, 2015

unsupported features from the r cookbook #161

Open

41 tasks

Fix annotations for facet_grid()

f800c12

Bump version, update NEWS, add Carson as author

f9e252a

merge with master

bfa5bfe

chriddyp added a commit that referenced this pull request Mar 11, 2015

Merge pull request #167 from cpsievert/carson-scales-free

d185f67

Carson scales free

chriddyp merged commit d185f67 into plotly:master Mar 11, 2015

cpsievert deleted the carson-scales-free branch March 13, 2015 01:55

Carson scales free #167

Carson scales free #167

Conversation

cpsievert commented Feb 18, 2015

cpsievert commented Feb 18, 2015

cpsievert commented Feb 18, 2015

mkcor commented Feb 18, 2015

mkcor Feb 18, 2015

Choose a reason for hiding this comment

cpsievert Feb 18, 2015

Choose a reason for hiding this comment

cpsievert commented Feb 18, 2015

chriddyp commented Feb 18, 2015

cpsievert commented Feb 18, 2015

tdhock commented Feb 18, 2015

tdhock commented Feb 18, 2015

cpsievert commented Feb 18, 2015

chriddyp commented Feb 18, 2015

tdhock commented Feb 19, 2015

chriddyp commented Feb 19, 2015

mkcor commented Feb 19, 2015

mkcor commented Feb 19, 2015

mkcor commented Feb 19, 2015

mkcor commented Feb 19, 2015

mkcor commented Feb 19, 2015

cpsievert commented Feb 19, 2015

mkcor commented Feb 20, 2015

mkcor Feb 20, 2015

Choose a reason for hiding this comment

tdhock commented Feb 25, 2015

tdhock commented Feb 25, 2015

cpsievert commented Feb 25, 2015

chriddyp commented Mar 9, 2015

cpsievert commented Mar 9, 2015

chriddyp commented Mar 9, 2015

cpsievert commented Mar 9, 2015

chriddyp commented Mar 9, 2015

cpsievert commented Mar 9, 2015

cpsievert commented Mar 9, 2015

cpsievert commented Mar 9, 2015

tdhock commented Mar 9, 2015

cpsievert commented Mar 10, 2015

tdhock commented Mar 10, 2015

cpsievert commented Mar 10, 2015

tdhock commented Mar 10, 2015

cpsievert commented Mar 10, 2015

cpsievert commented Mar 10, 2015

tdhock commented Mar 10, 2015