Test with optional cohort, defaulting to label cohort

[thcrock]

Trying out the concept of making the cohort default to the label in order to give beginners an easy way to specify the cohort+label in one go instead of splitting. I know there is a question about whether this is the best way to message the concept to the user, but this is a very easy way to start testing. The implementation really just consists of setting the cohort table name (which is sent to everything downstream) to the label table name in these cases.

The opposite (optional label config, otherwise use the cohort as label) is possible but requires adding and explaining two pieces of optional items to the cohort config: the optional outcome column, but also the optional {label_timespan} parameter in the query itself. I started off doing this but had a very hard time explaining how this would work in the example config file. If people feel strongly that this is the better route we can go that route.

But here I'd like to explore starting with this as an implementation, seeing if it works, and maybe even consider changing the names of some of these config keys if it makes things more clear.

Not all the tests run yet but I did verify the basic experiment tests run without a cohort config here.

[shaycrk]

Thanks @thcrock -- I think that makes a lot of sense as a starting point. My one main question about this approach in particular is what happens when multiple label timespans are specified in the config: won't this effectively result in duplicate rows in the cohort which would create problems downstream?

[thcrock]

@shaycrk Still some tests failing, particularly in test_partial_experiments. One of the failures there has me thinking if the test adds value. The GetSplits test asserts that you basically just pass an experiment the temporal config only, call 'run()', and expect it to not fail. It would just generate time chops for you to look at. Is this...a useful use case to support? The test fails here because the cohort generator itself has an assert after it runs saying 'if there are no rows, error'. It's hard to reconcile this assert with the GetSplits test case in my mind. What do you want Triage to do if the user just passes temporal config?

Otherwise, the strategy we discussed is implemented, so you could try it out if you'd like.

[shaycrk]

Thanks @thcrock -- that's a good question and I'm curious what @ecsalomon and @nanounanue think here, too.

I haven't personally made use of the partial experiment runs, though I have been wanting to build out some tools (along the lines of visualize_chops) to help users develop/debug different parts of the config file and could imagine making some use of the partial run functionality there, but really haven't thought it through enough yet to figure out the best approach. For instance, when I look at the time splits, I tend to just instantiate timechop directly rather than an experiment object.

I think if I had to choose now, I'd probably be fine with relaxing some of the partial run functionality here, but what do others think?

[shaycrk]

@thcrock -- looks like the python 3.9 test failures here are the same @tweddielin and I were debugging in #882. Not sure why it should suddenly be showing up now, but the solution seemed to be upgrading matplotlib to 3.3.4

[shaycrk]

Thanks @thcrock -- looks good overall! I had a couple small questions I noted inline, but seems pretty ready to merge otherwise.

@ecsalomon -- I think this might create a couple merge conflicts with #883 but hopefully should be fairly straightforward to resolve.

[thcrock]

@shaycrk Pushed code responding to three review comments and responded to the fourth

[shaycrk]

Looks good to me -- I'll go ahead and merge