Navigation Menu

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

Syntactic changes/additions: p_* and bf_* families #258

Closed
DominiqueMakowski opened this issue Nov 13, 2019 · 19 comments
Closed

Syntactic changes/additions: p_* and bf_* families #258

DominiqueMakowski opened this issue Nov 13, 2019 · 19 comments

Comments

@DominiqueMakowski
Copy link
Member

Currently, we have:

  • p_map
  • p_direction
  • p_significance
  • rope

which are similar in the sense that they represent some sort of proportion/probability.

First question, should we add an alias to rope as p_rope, to make the p_* family consistent and complete?

Also, we currently have the bayesfactor_* family, with bayesfactor_parameters being able to compute BFs against 0 (in a way, kinda close to p_map), against rope and potentially against direction and significance.

Hence, rather than having these options available as arguments of bayesfactor_parameters, what about exposing them as mirrors to the p_* family, in a bf_* family, with bf_direction, bf_rope, bf_significance and lastly bf_map (the "basic" BF against a point-null), which might not the most appropriate name (maybe something like bf_zero).

Thoughts?

(PS: I know that initially, I favoured bayesfactor_* over bf_*, but it seems like the latter shortcut has become super popular and accepted so I think I am changing my mind 😁)

@mattansb
Copy link
Member

1. Splitting

Dom, is that you??? Is it this the same person who originally wanted all the Bayes factor functions to be under one single bayesfactor function???

Within the bayesfactor_parameters there are basically 4 options, on 2 dimensions:

Point Null ROPE Null
Two Tailed Alternative
One Tailed Alternative

Internally, there is only one function, so we would essentially be making wrap-functions to split the functionality up? If we do this, I would suggest (as I originally did) adding (not replacing) only two functions, by the null type: bayesfactor_savagedicky (bring back from the dead) and bayesfactor_rope (or their bf_* names), so we would have, in total:

  • _parameter
    • _savagedickey
    • _rope
  • _models
  • _inclusion
  • _restrict

On the other hand / additionally - p_direction, p_significance, rope share a basic core (that is not shared with p_map) - they are all posterior probabilities. Perhaps they can be unified under some general function, p_posterior (?), which can also allow users to input any range? (so users can ask, e.g., what is the probability of the parameter being between c(12, 45)?). What do you think?

Now the p_map is a confusing name IMO. Is the p_ only because of the "p" in p-value?

with bayesfactor_parameters being able to compute BFs against 0 (in a way, kinda close to p_map),

I feel triggered....

2. Renaming

Would the bf_* be replacing the bayesfactor_* versions? Or just added aliases?
Note that for the master bayesfactor function, bf would conflict with brms::bf.

@strengejacke
Copy link
Member

  1. To avoid confusion, from a user's perspective, I would - as @mattansb suggested - rather add bf_rope() etc. as additional aliases, not replace existing functions names.

  2. We could, furthermore, add aliases with bf_*-prefix to the bayesfactor_* functions. I would probably not replace bayesfactor_* with bf_*.

  3. p_significance() or p_direction() return a probability value - that's why these have the p_* prefix. rope(), however, does not. I don't see any conceptional conflicts here. p_posterior() might be misleading, as we don't compute a probability value for the posterior - whatever that would be?

@mattansb
Copy link
Member

Sorry, right, only rope(..., ci = 1) returns a posterior probability.

@DominiqueMakowski
Copy link
Member Author

DominiqueMakowski commented Nov 17, 2019

Agree with the aliasing.

If I understand you, we'd have:

  • Against point null:
    • p_map (which is prob at MAP vs. prob at 0)
    • bf_zero (= bf_savagedickey)
  • Against ROPE
    • p_rope (= rope(..., ci = 1))
    • bf_rope
  • Against direction
    • p_direction
    • bf_direction
  • Against region of practical significance (i.e., ROPE * direction)
    • p_significance
    • bf_significance
  • Other BFs
    • _models
    • _inclusion
    • _restrict
  • Other indices
    • rope
    • equivalence_test

and bf_* would also be bayesfactor_*

@strengejacke
Copy link
Member

Looks good, and we should somehow make this "overview" prominent in the docs/vignettes. Once users grasp the conceptional differences, it's easier to follow the decision for naming functions.

@mattansb
Copy link
Member

@DominiqueMakowski as I noted above, I think we can do with only 2 bf_* functions for parameters: bf_savagedickey and bf_rope, with both supporting a directional option (I feel like 4 is over-kill and would be more confusing than helpful).

So hope about:

  • For parameter, bf_ / bayesfactor_parameters with two aliases:
    • bf_ / bayesfactor_savagedickey (I don't like bf_zero ☹)
    • bf_ / bayesfactor_rope
      (That 6 function names that all call the same underlying function... seem like plenty, no?)
  • For the rest (bayesfactor_inclusion, bayesfactor_models and bayesfactor_restricted) add a bf_* alias.

mattansb added a commit that referenced this issue Nov 17, 2019
DominiqueMakowski added a commit that referenced this issue Dec 2, 2019
to make room for new p_rope #258

- It's an ugly name, but since this index is not relly used/advertised (it's mainly there for legacy reasons now), I guess it's okay (til eventually find some use for this thing)
DominiqueMakowski added a commit that referenced this issue Dec 2, 2019
@DominiqueMakowski
Copy link
Member Author

DominiqueMakowski commented Dec 2, 2019

I don't like bf_zero ☹

Why? Would bf_pointull be better? or bf_null?

It's just that "savage-dickey" is not really transparent, whereas above I was leaning toward a new framework of method-type_against-what. With 2 method types (BFs or "proportions of posterior") and several "against-what", direction, rope, sig, etc.

I feel like 4 is over-kill and would be more confusing than helpful

I am not sure about it. I think the theoretical overlaps and discrepancies would be more obvious with this new design. Currently, it seems like the BFs are completely different from the proportion tests, whereas in fact it appears to me now that it is all a matter of perspective (method: BF or posterior proportions) on a given hypothesis (something vs. something else). (not talking here about the other BFs such as inclusions and restricted) Reflecting this in the functions could make things easier, and it was probably because this wasn't clear to me at first that I thought it would be a good idea to have only one bf() function ☺️

Bayesian tests

  • Against point null:
    • p_map (which is prob at MAP vs. prob at 0)
    • Should we alias p_map with p_zero or p_pointnull (to be aligned with bf and also consistent with the "method_against" interface?
    • bf_pointnull (= bf_savagedickey)
  • Against ROPE
    • p_rope (= rope(..., ci = 1))
    • bf_rope
  • Against direction:
    • p_direction
    • bf_direction
  • Against region of practical significance (i.e., ROPE + direction):
    • p_significance
    • bf_significance
  • Other BFs:
    • bf_models
    • bf_inclusion
    • bf_restricted
  • Other posterior proportions:
    • rope
    • equivalence_test

@mattansb
Copy link
Member

mattansb commented Dec 2, 2019

Okay, you broke me - I will add bf_pointull.

Currently, it seems like the BFs are completely different from the proportion tests, whereas in fact it appears to me now that it is all a matter of perspective.

They are compactly different! They ask completely different questions - the BF asks questions about the data (what did we learn from the data) whereas posterior estimates (and their proportion tests) ask questions about the posterior. Their relationship to the priors are also different (as we will soon show in our upcoming paper). But all this is somewhat besides the point here (kind of)...

The main reason we shouldn't have separate functions for bf_direction and bf_significance is that by having functions that select the direction based on the posterior (like p_direction and p_significance do), you are changing the priors used in the computation of the BF (as the "directionality" of the BF is done by placing an order restriction on the prior) - that means that the prior is changed post-hoc, which is a big no-no! Richard Morey explicitly warns against improper use of BF in exactly this manner. I see no reason we should then offer this option...

Additionally, I think it is a disservice to users, to whom having there "separate functions" makes it seem like these are different things (and might also encourages bf-hacking, by running all functions and seeing which is the "best").

Honestly, the more I think about it, even bf_rope seems like a bad idea (as users should always define their own rope).

mattansb added a commit that referenced this issue Dec 2, 2019
@DominiqueMakowski
Copy link
Member Author

you are changing the priors used in the computation of the BF

True...

Additionally, I think it is a disservice to users [...] might also encourages bf-hacking

good point... Although I am afraid if people want to hack they will find a way no matter what, we cannot be the ultimate gatekeepers of good practices here

Again, I favour leaving all options open for the users, but accompanied with documentation of good practices, - for them to understand why some methods are worse than others-, rather than enforcing some practices and restricting freedom, because the user will find what he's looking for anyway through digging in or in other packages.

even bf_rope seems like a bad idea

we opened the pandora's box 🙉

Okay, let's add bf_pointnull (why not ZERO then?? 😅) and alias its counterpart p_pointnull, and we'll leave it at that?

@mattansb
Copy link
Member

mattansb commented Dec 2, 2019

Alright 😁 - I've replaced bf_savagedickey with bf_pointnull (I don't like bf_zero because the null doesn't have to be 0 - e.g., in a proportions test it would most likely be 0.5).

@DominiqueMakowski
Copy link
Member Author

because the null doesn't have to be 0

Good point!

I've replaced bf_savagedickey with bf_pointnull

Replaced might to brutal, maybe we could either alias it and for now just deprecate bf_sd?

@mattansb
Copy link
Member

mattansb commented Dec 3, 2019

bf_sd has been deprecated for the last few releases (in favor of bf_parameters), so I think its safe to remove completely. But it's your call (:

@DominiqueMakowski
Copy link
Member Author

then I agree, let's remove! (sorry I'm out of sync 😵, I was quite busy with some other stuff recently)

@strengejacke
Copy link
Member

Honestly, the more I think about it, even bf_rope seems like a bad idea (as users should always define their own rope).

I would not remove bf_rope(), as we just published a paper on this index... ;-)

@strengejacke
Copy link
Member

Can anyone check the state of this issue? Has everything been addressed in the dev-branch?

@mattansb
Copy link
Member

mattansb commented Dec 5, 2019

All is done on the BF end 👍

@strengejacke
Copy link
Member

@DominiqueMakowski bump

@strengejacke
Copy link
Member

maybe I should use some animations here to gain more attention...

ZsaDfKR

@DominiqueMakowski
Copy link
Member Author

DominiqueMakowski commented Dec 9, 2019

don't you see I'm busy creating some awesome animations?! don't have time for minor details such as the API of the package 😁😁

But I think everything's in order, we just need indeed to summarise this in the vignette #262

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

No branches or pull requests

3 participants