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

dev #201

Merged
merged 22 commits into from
Jul 30, 2019
Merged

dev #201

merged 22 commits into from
Jul 30, 2019

Conversation

DominiqueMakowski
Copy link
Member

@DominiqueMakowski DominiqueMakowski commented Jul 30, 2019

Try addressing the reviewer's comments:

Reviewer 2

Thanks a lot for these comments @tjmahr ! We will start addressing them in this PR ☺️

Features

  • The ROPE procedure and other indices use the highest density interval. Is there any option to use an equal-tailed interval?

We added a ci_method argument in rope() to allow for ETI to be used.

  • density_at() isn't doing computing a probability. I would remove estimate_probability() and probability_at() because they are just aliases for density functions and density is the more appropriate term.

We removed the two aliases with probability. We also clarified in the documentation that it is pertaining to the value of the density function.

README

  • I do not see anything for contribution guidelines. I would add a CONTRIBUTING.md file.

We added a contributing file.

  • I would include the text output of the R code [in the README].

We have additionally included the text output from the R Code in the README.

  • I would also note that the figures there are diagrams meant to illustrate the statistical concept.

We have added a sentence to point out that these figures are meant to illustrate the statistical concepts, and pointed the readers to the see-package, where plotting-methods are provided:

"The following figures are meant to illustrate the (statistical) concepts behind the functions. However, for most functions, plot()-methods are available from the see-package."

  • I don't see a demo for eti().

We have added a demo for eti() to the README.

  • "Moreover, 89 is the highest prime number that does not exceed the already unstable 95% threshold (McElreath, 2015)." The primeness of 89 is not important. McElreath's choice of 89 in Statistical Rethinking text was to illustrate that interval widths are arbitrary and that there is nothing special about 95 or 90 compared to 89.

We have rephrased the sentence to emphasize the idea behind choosing the 89 as CI-level:

"Moreover, 89 indicates the arbitrariness of interval limits - its only remarkable property is being the highest prime number that does not exceed the already unstable 95% threshold (McElreath, 2015)"

Furthermore, although already implied in the paper, we also emphasized the point of arbitrariness on the paper as well.

  • "equivalence_test() a Test for Practical Equivalence based on the" Needs a verb.

We added a verb to the sentence:

equivalence_test() is a Test for Practical Equivalence based on the...

  • I don't understand the Bayes Factor diagram in the README.

We have added a paragraph to explain the figure more in detail:

The lollipops represent the density of a point-null on the prior distribution (the blue lollipop on the dotted distribution) and on the posterior distribution (the red lollipop on the yellow distribution). The ratio between the two - the Svage-Dickey ratio - indicates the degree by which the mass of the parameter distribution has shifted away from or closer to the null.

  • "a range of -0.05 to -0.05." This range is the same number twice.

Thanks, we fixed the typo!

  • "Savage-Dickey density ratio is computed" Should this have a reference?

Thanks, we have added a reference, and furthermore added a reference-list to the end of the README.

  • I don't see a demo for the area under the curve functions.

See comment from TJ below, no longer necessary.

Documentation

  • ROPE-based p documentation: I don't understand how a p-value can get a negative percentage. What would a 0% p-value mean? If this index doesn't act like a familiar p-value, it is probably the wrong name for it.

We have clarified the documentation of this index and underlined its exploratory nature. We also made clear that the negative sign reflects the direction of the index (wether in corresponds to significance or non-significance), rather than actual negative probabilities, which indeed make no sense.

The ROPE-based \emph{p}-value is an exploratory and non-validated index representing the 
maximum percentage of \link[=hdi]{HDI} that does not contain (or is entirely contained, in which 
case the value is prefixed with a negative sign), in the negligible values space defined by the 
\link[=rope]{ROPE}. It differs from the ROPE percentage, \emph{i.e.}, from the proportion of a given
 CI in the ROPE, as it represents the maximum CI values needed to reach a ROPE proportion of 0\% 
or 100\%. Whether the index reflects the ROPE reaching 0\% or 100\% is indicated through the sign:
 a negative sign is added to indicate that the probability corresponds to the probability of a not 
significant effect (a percentage in ROPE of 100\%). For instance, a ROPE-based \emph{p} of 97\% 
means that there is a probability of .97 that a parameter (described by its posterior distribution) is 
outside the ROPE. In other words, the 97\% HDI is the maximum HDI level for which the percentage 
in ROPE is 0\%. On the contrary, a ROPE-based p of -97\% indicates that there is a probability of .97 
that the parameter is inside the ROPE (percentage in ROPE of 100\%). A value close to 0\% would 
indicate that the mode of the distribution falls perfectly at the edge of the ROPE, in which case the 
percentage of HDI needed to be on either side of the ROPE becomes infinitely small. Negative values 
do not refer to negative values \emph{per se}, simply indicating that the value corresponds to non-
significance rather than significance.

Paper

  • The first mention of bayestestR in the second paragraph is awkward. Specifically, the text shifts from talking about common ways to describe effects in a Bayesian framework to talking about the features of the package: "Additionally, bayestestR also focuses on implementing a Bayesian null-hypothesis testing framework ..."
  • "However, bayestestR functions also include plotting capabilities via the see package (Lüdecke, Waggoner, Ben-Shachar, & Makowski, 2019).": I don't see any plotting examples in the README or documentation pages. I see plotting methods in the NAMESPACE.
  • I think it would worthwhile to demonstrate that the functions demoed in the article also work on models. For example, I can call p_direction() and bayesfactor_parameters() directly on a model and get the results for each parameter.

Proofreading

  • Every reference of Kruschke spells out the author's full name.

Hopefully fixed (changed the name in the .bib file). However, I am not sure why would that happen. One possible reason is disambiguation, yet all instances were written the same way...

  • Figure 2 should be referenced in the text.
  • "(i.e. the difference": Needs a comma.
  • "The Bayesian framework allows to neatly delineate": Allows one.
  • developped
  • Nevertheless, in the absence of user-provided values, bayestestR will automatically find an appropriate range: Nevertheless doesn't make sense.
  • bases on prior and posterior samples: Based
  • The system for building the references section should protect some words from being converted to lowercase. (In LaTeX, this is done with {}). Right now, for example, it says Brms: An r package for bayesian multilevel models using stan but I would make sure that the system produces brms: An R package for Bayesian multilevel models using Stan.

Typos have been fixed.

@codecov-io
Copy link

codecov-io commented Jul 30, 2019

Codecov Report

Merging #201 into master will decrease coverage by 0.02%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage    68.8%   68.77%   -0.03%     
==========================================
  Files          46       46              
  Lines        2170     2178       +8     
==========================================
+ Hits         1493     1498       +5     
- Misses        677      680       +3
Impacted Files Coverage Δ
R/p_map.R 80.35% <ø> (ø) ⬆️
R/p_rope.R 82.92% <ø> (ø) ⬆️
R/estimate_density.R 43.63% <ø> (ø) ⬆️
R/plot.R 0% <0%> (ø) ⬆️
R/map_estimate.R 85.71% <100%> (+1.93%) ⬆️
R/bayesfactor_parameters.R 78.53% <100%> (ø) ⬆️

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 a26ee76...a40e630. Read the comment docs.

@DominiqueMakowski
Copy link
Member Author

DominiqueMakowski commented Jul 30, 2019

  • For the README, I would tend to replace most of it with the paper... The figures are more concise there. what do you think?

@mattansb
Copy link
Member

"Savage-Dickey density ratio is computed" Should this have a reference?

Hmm.. The reference is at the end of that sentence. 🤷‍♂️

@strengejacke
Copy link
Member

"Savage-Dickey density ratio is computed" Should this have a reference?

Hmm.. The reference is at the end of that sentence. 🤷‍♂️

In the paper, not in the readme.

@mattansb
Copy link
Member

In the paper, not in the readme.

Done!

@strengejacke
Copy link
Member

I don't see a demo for the area under the curve functions.

I would say that we don't need a demo here, because this function is not specifically related to Bayes stuff, but also useful in other contexts. It's just that we needed it in bayestestR?

@strengejacke
Copy link
Member

The ROPE procedure and other indices use the highest density interval. Is there any option to use an equal-tailed interval?

Currently not, but we may address this in a future update (thus, file an issue for now). Would this be sufficient?

@DominiqueMakowski your final turn. :-) I personally would not make changes (for now) for the two remaining issues, just respond and give reasons why (see my opinion).

@tjmahr
Copy link
Contributor

tjmahr commented Jul 30, 2019

Got it. Demo of AUC stuff is not necessary.

@DominiqueMakowski
Copy link
Member Author

Currently not, but we may address this in a future update

Was quite straightforward to implement (added for rope). @strengejacke what do you think?

Well I gotta say these few hours revealed a neat and efficient collaboration ☺️ good job guys thanks everyone.

I think we are mostly done... In this case, I'll copypasta our answers on the JOSS issue as our official response

@strengejacke
Copy link
Member

what do you think?

Looks good! And indeed easier than I first thought.

I think we are mostly done... In this case, I'll copypasta our answers on the JOSS issue as our official response

image

@strengejacke
Copy link
Member

Should we merge this PR in order to generate the updated PDF in the JOSS-issue?

@DominiqueMakowski
Copy link
Member Author

I suggest we merge right after Travis greenlight 😁 (in a few minutes)

@strengejacke
Copy link
Member

36u6lk

@strengejacke
Copy link
Member

Travis is broken, so >>> merge!
(I would rather check on Linux than OS X, as Travis Mac checks have been unreliable for some time now...)

@DominiqueMakowski
Copy link
Member Author

image

Let me see how I can please travis

@strengejacke
Copy link
Member

strengejacke commented Jul 30, 2019

image

@DominiqueMakowski
Copy link
Member Author

see ☺️

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.

5 participants