Skip to content

Support date-time binning in histograms #151

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 4 commits into from
Dec 30, 2014
Merged

Conversation

mkcor
Copy link
Contributor

@mkcor mkcor commented Dec 30, 2014

When date-time durations are expressed with a numerical value, plotly's unit is the millisecond (ms).
This is different in R, where the POSIXt class uses seconds, and the Date class uses days.

This PR offers the corresponding conversions for the width (size) of (x) bins in histograms.

See these successful ggplotly conversions when dates are of class POSIXt:
https://plot.ly/~marianne2/336/deaths-by-police-across-time-by-region/,
when dates are of class Date:
https://plot.ly/~marianne2/358/deaths-by-police-across-time-by-region/

/cc @chriddyp @alexcjohnson @jackparmer

@mkcor
Copy link
Contributor Author

mkcor commented Dec 30, 2014

New images can be seen here: 18709da
(Corresponding to b290026)

@mkcor mkcor changed the title Marianne datetime binning Support date-time binning in histograms Dec 30, 2014
@@ -11,7 +11,7 @@ test_that("default position is translated to barmode=stack", {
expect_identical(L$kwargs$layout$xaxis$type, "category")
expect_identical(L[[1]]$type, "histogram")
expect_true(L[[1]]$x[1] %in% c("CDN", "MEX", "USA"))

Copy link
Collaborator

Choose a reason for hiding this comment

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

whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, actually: indentation. This is the convention / automatically added in RStudio.
Both conventions (A: preserving indentation within a block and B: no indentation on empty lines) make sense, I once found a great thread on the topic on StackOverflow... Let me see if I can find it again.

@alexcjohnson
Copy link
Collaborator

Aside from the 3 lines with added whitespace, looks great!

@mkcor
Copy link
Contributor Author

mkcor commented Dec 30, 2014

@alexcjohnson This one, I think: http://stackoverflow.com/questions/2727988/python-indentation-in-empty-lines
I like all these points made by all the people... I really like that A is copy-pastable in an interpreter.
And most R users are RStudio users.

@alexcjohnson
Copy link
Collaborator

If that's a standard R convention I guess we can keep it in this repo. In the rest of our code though we have always deleted trailing whitespace. I see the argument about python interpreters, but I think this has too much weight behind it and it keeps diffs much cleaner to insist on no trailing whitespace.

@mkcor
Copy link
Contributor Author

mkcor commented Dec 30, 2014

Yes, I know. You probably even have a git alias for forcing trailing whitespace removal upon every commit, don't you? ;)

Anyway, it's a cultural thing and I wasn't trying to contaminate the rest of our code base. ;)

mkcor added a commit that referenced this pull request Dec 30, 2014
Support date-time binning in histograms
@mkcor mkcor merged commit 7436af9 into master Dec 30, 2014
@mkcor mkcor deleted the marianne-datetime-binning branch December 30, 2014 22:45
@alexcjohnson
Copy link
Collaborator

@mkcor Even farther back than git - I have my editor set to strip trailing whitespace on every save. So I personally would have to override my editor settings (or just use RStudio) in order to work in this repo, or use something like http://stackoverflow.com/questions/3515597/git-add-only-non-whitespace-changes (wow, people really get riled up about this issue!).

Anyway, the only reason I care about this is having to look at diffs with whitespace-only changes. As long as all contributors here agree to the same convention (looks like the blank lines were added by @chriddyp ) it's fine.

@mkcor mkcor mentioned this pull request May 5, 2015
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