Closed beanumber closed 7 years ago
I think we've now addressed the majority of the issues you raise -- it's definitely cleaned up relative to the first draft. Let us know if anything still stands out to you.
First of all, I'm embarrassed that it has taken me this long to respond. Please accept my apologies!
The revision is certainly an improvement. In particular, I think the end is much stronger with the concluding paragraphs. I still think there is too much code, but I guess that decision should be made by @jennybc and @hadley.
A few general comments:
for
loops with calls to apply()
, which is good. I still see places where further clarity could be achieved. For example, in plot_distribution_of_movement_distances()
couldn't the for
loop be replaced by a call to lag()
? Anyway, I realize it's ridiculous to expect a full code re-write, but others may spot similar things. Some minor comments:
a
and h
stand for away
and home
.add_possession_columns_to_moments()
$TEAM_ID
means. Thanks for contributing this paper -- it was my pleasure to read!
Thanks @beanumber for the continuing review!
I couldn't resist chiming in here. I've been a big proponent that data are plural. But I should note that the Wall Street Journal and others think differently:
http://blogs.wsj.com/economics/2012/07/05/is-data-is-or-is-data-aint-a-plural/
One of my New Year's resolutions is to be more relaxed about English grammar.
Just my $0.02.
On Jan 31, 2017, at 3:42 PM, Ben Baumer notifications@github.com wrote:
• "data" is plural
Nicholas Horton Professor of Statistics Department of Mathematics and Statistics, Amherst College PO Box 5000, AC #2239 Amherst, MA 01002-5000
Thanks for the detailed review! I'm working on addressing your comments right now.
RE: one-hot encoding, I've seen it used in the literature with a hyphen. Looking it up online I see some with it (like the Wiki page) and some without.
I really like what you are trying to do here. Your work on this topic has been really well-received, and I think a lot of people will appreciate the kind of nuts-and-bolts how-to that you present here. The concept of starting with the raw data, discussing some of the wrangling challenges, briefly discussing previously-published models, creating interesting visualizations, and discussing results, is perfect. I do have comments on the implementation. :)
Since you have included so much code in the paper (roughly half of the content) you should think carefully about what you show here and what you hope to accomplish. I think there is too much code. While the functions are well-documented, many of them are too long and not that interesting. The code itself is also written in a non-R style with lots of
for
loops and indexing. I wonder whether much of this code could be greatly shortened by using adplyr
approach. For example, the first function you present isadd_possession_columns_to_moments()
, and this occupies more than a full page. Can't this be written as a single call tomutate()
with someifelse()
statements? If all you are doing is defining two new columns based on the corresponding values of existing columns, then I think this should work. Similarly, inget_moments_w_possession_cols()
I wonder if an appeal todplyr::do()
would be a conceptually simpler way to go here.This raises the larger question of what you hope to accomplish by including the code for these functions. If the goal is simply to be explicit about exactly what you have done, then you have certainly achieved this. However, I don't know how many people will want to sit down and read this code. You might reach a wider audience by putting the code in an R package, documenting the functions there, and then using the space you free up to illustrate how the functions work and how another analyst might use them. You would also have more room for discussing interesting findings. [A
sportVU
package for R would be pretty exciting!]I'll leave it to the @jennybc and @hadley to decide whether adding two derivative columns to a data frame is an operation that will be interesting to readers. As a data scientist, I think it's cool, but then it invites me to examine your code and think that it could be done more simply. As a basketball fan, it's not really that interesting.
xtable
. Since the variables relating to the 10 players are all the same, you could condense this by just showing the non-player related variables, and one set of the four variables describing the i-th player.In summary, I think this is a really worthwhile effort, but that it would work better as a vignette about what the
sportVU
package can do and how it works, as opposed to an annotated description of your code.