242 quantile renaming

[dajmcdon]

closes #242

[dshemetov]

Our meeting notes say we were going to use the term quantile_level. Want to make sure that we're consciously switching to quantile_value instead. I think level better evokes the fixed values of a categorical variable. I do wish there was a more standardized term for the thing we're talking about here, but other packages (like numpy) don't offer better solutions.

[dsweber2]

let's see, the variables that were renamed were:

• levels
  There's a couple of places with levels still present as a variable:
  

  epipredict/R/epi_juice.R

  Line 29 in 07600a2

  if (!is.null(object$levels)) {

  
  

  epipredict/R/epi_recipe.R

  Line 125 in 07600a2

  levels = union(

  
  

  epipredict/R/bake.epi_recipe.R

  Line 89 in 07600a2

  # The levels are not null when no nominal data are present or

  
  looking at the set, I think this might just be a clobbered variable name.

• tau
  several of these lurking in make_smooth_quantile_reg and make_quantile_reg
  

  epipredict/R/make_smooth_quantile_reg.R

  Line 127 in 07600a2

  original = "tau",

  
  

  epipredict/R/make_quantile_reg.R

  Line 107 in 07600a2

  dist_quantiles(x, list(object$tau))

  
  I'm guessing functions like tau_duplication and maybe all the taus in dist quantiles are left intentionally?

• q
  both q and p are pretty hard to search for so I may have missed a couple. this function and the one below it both use q internally
  

  epipredict/R/dist_quantiles.R

  Line 33 in 07600a2

  q <- field(x, "values")

  
  another pair down a ways in the same file
  

  epipredict/R/dist_quantiles.R

  Line 129 in 07600a2

  q <- quantile(x, probs, ...)

  
  this bit of docs
  

  epipredict/R/dist_quantiles.R

  Line 155 in 07600a2

  #' edf$q <- dist_quantiles(list(1:5, 2:4, 3:10), list(1:5 / 6, 2:4 / 5, 3:10 / 11))

  
  actually shows up quite a bit in this file; I suppose given that it's all specifically about quantiles it's probably a more reasonable use.
  This seems like it might actually need to change (also on 127 below that)
  

  epipredict/R/layer_residual_quantiles.R

  Line 119 in 07600a2

  q = list(quantile(

• p
  also quite difficult to search for. I don't think I found any that are probabilities; there's some predictors in examples named p, but that's not caught in this refactor. There's this anonymous function, which seems fine
  

  epipredict/R/make_smooth_quantile_reg.R

  Line 174 in 07600a2

  list_of_pred_distns <- lapply(x, function(p) {

  
  So dist_quantiles has a number that I think might need to change:
  https://github.com/cmu-delphi/epipredict/blob/242-quantile-renaming/R/dist_quantiles.R#L48
  this one definitely needs to change
  https://github.com/cmu-delphi/epipredict/blob/242-quantile-renaming/R/dist_quantiles.R#L123
  lines 334, 338, 342, 345, 358, 363, 364, 365, 376 all seem like reasonable local variables (they're short enough functions that 1 letter variables is probably not the worst thing in the world).
  I'm assuming we don't have doctests or something like that? This one definitely needs to change
  

  epipredict/vignettes/epipredict.Rmd

  Line 168 in 015b0ea

  head(quantile(out_q$predictions$.pred_distn, p = .4))

The search I ended up using for the single letters was [^a-zA-Z_\-0-9]p[^a-zA-Z_\-0-9], which I think gets rid of all the places that p can be part of another word or variable?

[dajmcdon]

Hmmm, nuts. That's not what I listed in #242. But my memory is hazy. Does quantile_level sound right to the rest of you?

[dsweber2]

I think that having both values and quantile_values is kind of confusing; I don't remember what we said in the meeting but quantile_levels seems less likely to get clobbered when I'm working quickly.

[dajmcdon]

I think some of these actually have to stay.

• levels this is referring to levels of a factor.
• tau these are the underlying arguments of functions from other packages. This syntax actually maps our new names for these things to those arguments. So these stay.
• q I think these are all local. I'm happy to try to change them, but I don't think it will matter.
• p These are a bit stupid. In multiple cases, I used partial argument matching. The generic quantile() function takes probs as an argument. So I left probs as an arg to extrapolate_quantiles() which is basically trying to take quantiles of a dist_quantiles object. But you can match the argument as quantile(x, p = z) which internally becomes quantile(x, probs = z). This is probably not optimal coding practice. I'll try to fix these.

[dsweber2]

• good to know, I just wanted to confirm for both the levels and the tau
• it might be good for consistency's sake to rename the q's, but I don't have strong feelings
• oh right, partial matching. Thanks for ditching those, they keep causing spurious warnings. Definitely an R footgun imo

[dajmcdon]

Ok. I think I fixed all the quantile_values -> quantile_levels.

I was wrong about p being a partial match. In the {distributional} package that we Import for this purpose, it actually defines the quantile method as quantile(x, p, ...) rather than quantile(x, probs, ...). So we're stuck with that one.

[dshemetov]

Ask distributional maintainer to switch the naming?

[nmdefries]

Found a couple vars in dist_quantiles.R that may need to be updated, I couldn't tell if David's previous comments addressed these.

[nmdefries]

Does tau_out need to be changed too, e.g. tail_extrapolate line 277?

Also a lot of probs here in extrapolate_quantiles and variants.

p and q used as args for functions starting on line 87, in exp_q_par, exp_tail_q, qlaplace, norm_q_par, norm_tail_q. (Searched using regex [^_a-zA-Z0-9]p[^_a-zA-Z0-9]).