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

refactor mcmc_intervals() and mcmc_areas() #115

Merged
merged 28 commits into from Sep 21, 2017

Conversation

@tjmahr
Copy link
Collaborator

commented Sep 20, 2017

Closes #103.

Advances #97 (add _data() functions).

These plots now use a discrete y-axis. Previously, we used a continuous y-axis, but relabeled the integer labels with parameter names. This caused some unexpected behavior.

We also use ggridges to draw the density curves in order to keep a discrete y-axis. One possibly unexpected implementation detail is that the point estimate in mcmc_areas() is no longer a vertical line segment, but a very narrow interval drawn by ggridges as well.

The main visual differences are:

  • these plots no longer have a "forehead" for an extra step on the y-axis. (This could in principle be restored)
  • that the density curves are not normalized to have the same height.

Previously, a flat, thin density curve would be stretched vertically and look like a wall.

image

Now it looks more like a flat density curve and the spikes in the other parameters look like spikes.

image

(Of course, neither of these plots is any good, because of the variables being plotted.)

Visual diffs

These are the plots in the examples.

Current
01-current
Update
01-updated


Current
02-current
Update
02-updated


Current
03-current
Update
03-updated


Current
04-current
Update
04-updated


Current
05-current
Update
05-updated


Current
06-current
Update
06-updated


Current
07-current
Update
07-updated


Current
08-current
Update
08-updated


Current
09-current
Update
09-updated


Current
10-current
Update
10-updated


Current
11-current
Update
11-updated

@tjmahr

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2017

Hmm, we need a bit of forehead back or we need to slightly tweak the vertical scaling. I'm noticing some of the areas get truncated.

image

Copy link
Collaborator Author

left a comment

oops, I thought I was commenting on a code chunk.

@tjmahr

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2017

Okay, slightly expanded y-axis for mcmc_areas.

image

@codecov-io

This comment has been minimized.

Copy link

commented Sep 20, 2017

Codecov Report

Merging #115 into master will decrease coverage by 0.09%.
The diff coverage is 98.67%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #115     +/-   ##
=========================================
- Coverage    99.6%   99.51%   -0.1%     
=========================================
  Files          30       30             
  Lines        3816     3881     +65     
=========================================
+ Hits         3801     3862     +61     
- Misses         15       19      +4
Impacted Files Coverage Δ
R/mcmc-diagnostics.R 99% <100%> (+0.02%) ⬆️
R/helpers-gg.R 95.45% <80%> (-4.55%) ⬇️
R/mcmc-intervals.R 98.99% <98.92%> (-1.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 492e91f...67f0993. Read the comment docs.

@jgabry

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

DESCRIPTION Outdated
rlang
rlang,
tidyr,
ggridges

This comment has been minimized.

Copy link
@jgabry

jgabry Sep 20, 2017

Member

I'm always hesitant to add more dependencies, especially when only used in one or two places. Do you think tidyr will be useful for other stuff going forward too? If so then I think it's fine to add the dependency now, but if not then is it possible to implement this without tidyr?

This comment has been minimized.

Copy link
@tjmahr

tjmahr Sep 20, 2017

Author Collaborator

I think I figured out a way to do it without tidyr.

This comment has been minimized.

Copy link
@tjmahr

tjmahr Sep 20, 2017

Author Collaborator

Got it

This comment has been minimized.

Copy link
@jgabry

jgabry Sep 20, 2017

Member

Thanks. I'm totally fine with using tidyr now if you think you'll probably want to use it a lot going forward. If not then I think the way you found of doing it without tidyr looks fine. Sorry if it was a pain to figure out.

geom_ignore <- function(...) {
geom_blank(mapping = NULL, data = NULL,
show.legend = FALSE, inherit.aes = FALSE)
}

This comment has been minimized.

Copy link
@jgabry

jgabry Sep 20, 2017

Member

The geom_ignore wrapper is a nice idea.

point_args$fill <- get_color("l")
}
# Don't import `filter`: otherwise, you get a warning when using
# `devtools::load_all(".")` because stats also has a `filter` function

This comment has been minimized.

Copy link
@jgabry

jgabry Sep 20, 2017

Member

Yeah that's annoying. It'd be nice to import filter from dplyr since we use it a lot but I think it would require importing only what we use from stats as opposed to just having @import stats. We could potentially do that.

@tjmahr

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2017

compute_column_density() is kind of intense with its nonstandard evaluation, but it supports arbitrary groups. E.g., for ridgelines... 😎

library(ggplot2)
library(ggridges)
devtools::load_all(".")

chains <- example_mcmc_draws() %>%
  prepare_mcmc_array() %>%
  melt_mcmc() %>%
  compute_column_density(c(Parameter, Chain), Value) %>%
  dplyr::filter(Parameter != "alpha", Parameter != "sigma")

ggplot(chains) +
  aes(x = x, y = Parameter) +
  geom_density_ridges(aes(height = density, color = factor(Chain)),
                      stat = "identity", fill = NA)

image

@jgabry
jgabry approved these changes Sep 20, 2017
@jgabry jgabry merged commit 7174d22 into stan-dev:master Sep 21, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.