MUCollective / multiverse

R package for creating explorable multiverse analysis
https://mucollective.github.io/multiverse/
GNU General Public License v3.0
62 stars 5 forks source link

Make option names not be list columns in multiverse table #34

Closed mjskay closed 5 years ago

mjskay commented 5 years ago

It's hard to read.

Might need to check that all option names are same type when created and throw an error if not.

abhsarma commented 5 years ago

To do this, we will have to have all option names be of the same character type. If they are declared, we can assert that.

However, if we have unnamed option types ( #1 ) :

branch(
  some_option,
    0,
    3,
    "abc",
    x + 5
)

Should this be equivalent to:

branch(
  trim,
    "0" ~ 0,
    "3" ~ 3,
    "abc" ~ "abc",
    "x + 5" ~ x + 5
) 

i.e. all option names are converted into characters? Or should this throw an error?

mjskay commented 5 years ago

Per our previous discussion I would vote throw an error. Otherwise it can lead to downstream errors that will be hard for users to diagnose.

So I would check that the type is unique and that it's one 9f the types we support (numeric, logical, character,...?) and if either check fails throw an error

abhsarma commented 5 years ago

so then we would enforce consistent types for unnamed branches as well. I feel like that might make using unnamed branches rather cumbersome. I mean it limits the scenarios where unnamed branches can be used and are useful.

For eg., Let's say I want to use a branch on filter, without specifying option names:

filter(df, branch(var1_filter_criteria,
    TRUE,
    var1 > 2
)
mjskay commented 5 years ago

Ah good point. Given use cases like that I could see an argument for coercing to character when types don't match. It might be worth the potential downstream errors (which should be rare anyway).

abhsarma commented 5 years ago

Cool. So explicit option names should be type consistent. While, derived option names will be coerced into character?

mjskay commented 5 years ago

For derived names I think I would coerce only if necessary. E.g. this:

branch(some_name, 0, 1, 2, 3)

should still end up with numbers.

mjskay commented 5 years ago

(also at that point i'd say keeping named options type consistent might be an optional feature, and just use the same coercion-only-if-types-different approach there for consistency)