add tests for morphology

[roblanf]

@wrightaprilm, can you take a look at what tests we should implement for the morphological stuff? I know you wrote a lot of extra tests, and in general we want all of them. However, the morph stuff in PF2 is a little different from what you had, mostly because we use entropies, and there are some default settings for kmeans that are now different from the ones in your version.

The best thing to do is fork the feature/morph2 branch, add the tests you want from your own branch, make sure they all work fine, and then submit a pull request back to feature/morph2. Once we have a good collection of tests, I can go ahead an merge back to master.

For ease of merging, please don't change any code just now - there are a few active branches and it would be easy to get conflicts. If you have tests that fail, just leave them failing and put a note here with any clues you have about what the problem might be. I'll work on the code at this end.

[wrightaprilm]

And by morph2, you mean morphology2? I assume this is true, but just checking in case there's a branch that didn't get pushed.

[roblanf]

ack, sorry. It's feature/morph

When this is done, I'll clean up the mess of branches!

Rob

On 13 May 2016 at 02:08, April Wright notifications@github.com wrote:

And by morph2, you mean morphology2? I assume this is true, but just checking in case there's a branch that didn't get pushed.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/brettc/partitionfinder/issues/97#issuecomment-218805976

Rob Lanfear School of Biological Sciences, Macquarie University, Sydney

phone: +61 (0)2 9850 8204

www.robertlanfear.com

[wrightaprilm]

K. I'll pull the data off of the cluster tonight or tomorrow and batch it through.

[wrightaprilm]

The 'models=' specifier should be binary or multi, with the '--raxml' flag passed in, yes? The example on examples/morphology on feature/morph isn't recognizing multi as an option.

[roblanf]

'binary' or 'multistate'

I kept them longer just because that's less ambiguous.

I'll change the example on the morph branch and sync.

On 13 May 2016 at 11:07, April Wright notifications@github.com wrote:

The 'models=' specifier should be binary or multi, with the '--raxml' flag passed in, yes? The example on examples/morphology on feature/morph isn't recognizing multi as an option.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/brettc/partitionfinder/issues/97#issuecomment-218927560

Rob Lanfear School of Biological Sciences, Macquarie University, Sydney

phone: +61 (0)2 9850 8204

www.robertlanfear.com

[roblanf]

ok, example is changed.

Also, I do not have any model lists for morph data. Because the only difference between the two multistate models is whether or not to use the lewis ascertainment bias correction. This is something that needs to be decided a-priori, and not by an algorithm.

If folks have data that's a mixture of binary/multistate and/or with/without the need for ascertainment bias correction, all they need to do is run separate PF analyses.

[wrightaprilm]

OK, so that all seems to be working. will see how it goes!

[wrightaprilm]

one issue I can spot right off the bat is the minimum partition size - which is larger than a good chunk of the datasets. Other than that, running very smoothly.

[roblanf]

suggestions? We could have a different default for morphology. Or, we could just make it clear in the manual that folks can set this however they like with something like this:

--min-subset-size 10

If we set a default, what's a sensible number? I'm not used to these models...

[wrightaprilm]

I think manual is actually quite cool, and might be useful for molecular data, too. My inclination is two-fold:

1. Anything but AICc is going to oversplit for datasets at this scale, and a little extra control might be helpful. And even for non-molecular datasets, contrasting how the different -ICs split the data with different minimum sizes might be informative (i.e., do all three criteria converge on the same set when the minimum size is > 1/5 of the dataset - how small does the universe of sets have to be to cause -ICs of varying stringency to favor the same set?).
2. In these tiny datasets, something people might be interested in doing is identifying characters that might be contributing spurious signal to the overall set, rather than actually generating partitions to inform an analysis.

[roblanf]

OK, so we'll just add it to the manual. I'll add that as another issue. On your point 2, I'm not sure that the methods (tiger or entropy) really do that. I'd want to see a lot of simulations first... It's a cool idea though. See follow-up email...

[wrightaprilm]

It looks like all my tests are passing. So you want a pull just containing the tests? I'll try to get this handled today.

[roblanf]

WOOOOOO! Yes. A pull with all the tests, ideally so that if I call 'sh all_tests.sh' then they just run.

On 16 May 2016 at 23:56, April Wright notifications@github.com wrote:

It looks like all my tests are passing. So you want a pull just containing the tests? I'll try to get this handled today.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/brettc/partitionfinder/issues/97#issuecomment-219430994

Rob Lanfear School of Biological Sciences, Macquarie University, Sydney

phone: +61 (0)2 9850 8204

www.robertlanfear.com

[roblanf]

Hey @wrightaprilm,

Right now I get this:

tests/morphology/test_AIC.py::test_aic PASSED
tests/morphology/test_aicc.py::test_aicc FAILED
tests/morphology/test_bic.py::test_bic PASSED
tests/morphology/test_incorrectmat.py::test_incmat FAILED
tests/morphology/test_mat.py::test_mat PASSED
tests/morphology/test_modeloutput.py::test_model PASSED
tests/morphology/test_nsubs.py::test_nsets PASSED
tests/morphology/test_wrongmodel.py::test_mixed FAILED

Two things.

1. From memory I think you said that two test are expected to fail, not three. So there's something up there.
2. Can you set the tests up so that if you have an expected failure, the test passes. The way we usually do this is to run the analysis, then check the log file for a bit of the expected error message. Take a look at some of the other tests and you'll get the idea. That way we don't have to remember what tests are supposed to fail. More simply put - all tests should pass, but it's OK to have a test where the expected outcome is that the program spits an error and quits. Just test for the appropriate outcome.

[roblanf]

more info on tests. If you look at the tests/full_analysis/test_full.py you'll see an example, like this:

def test_rerun_pf_error(rerun_pf_error):
    full_path = os.path.join(HERE, rerun_pf_error)
    load_rerun(full_path)
    with pytest.raises(util.PartitionFinderError):
        main.call_main("DNA", '"%s"' % full_path)

If you need help, ask @brettc. He is the test master.

[roblanf]

oh, and also:

can you add tests for all of the available morphology models - it's nice to check that they all work, so I know when I break them. The list of models is in models.csv, but it's this:

BINARY+G
BINARY+G+A
MULTISTATE+G
MULTISTATE+G+A

If you can do a separate test for each of these, that would be rad.

[roblanf]

Finally, what should we do if a user supplies binary data and wants to use a multistate model, or vice versa? Can you see what happens when you do these two things, report back here, and tell me what you think we should do?

[wrightaprilm]

Three quick things:

1. Did we decide if we're keeping the asc correction as a flag passed via command line, or did you want to incorporate that some other way?
2. My test-AICC is passing - could you paste in the failure message you're getting?
3. Right now, PF allows you to test binary data with a multistate model, and vice-versa. I think the most efficient way to handle this is to put a switch in somewhere like:

if datatype == 'morphology': 
    if model = 'multistate+g': 
       if alignment.unique() >= 3: 
             pass 
       else: 
            raise some error 
    elif model = 'binary+g': 
        if alignment.unique() == 2: 
            pass 
        else: 
          raise some error 

but I'm not sure where the best place to add this would be.

[roblanf]

Hi April,

1. I just defined it in the models - if you look in the manual it should make sense. E.g. BINARY+G is the binary model, BINARY+G+A is the binary model with ascertainment bias correction. Does that make sense? Have I missed something?
2. I'll double check.
3. Good idea. The alternative is to call these different datatypes, and have PartitionFinderBinary.py, and PartitionFinderMultistate.py. Any thoughts?

[wrightaprilm]

1. I promise I know how to read. Got the tests working, and I'll send a PR ...
2. ....once I know what's up here and patch it.
3. I have a slight preference to just put in a switch, and really make it hard for people to run data under the wrong set of candidate models. But that also might be something people want to do (i.e., I think maybe there is a unobserved third or fourth state for some of my characters, what would model selection look like if this were true?). So I'm fairly ambivalent on how this is done. Whatever seems most conceptually simple is fine.

[roblanf]

Hi @wrightaprilm,

All good. I fixed the broken test (missing folder). I also deleted the morph branch and merged it into master.

So, if you could:

1. Pull in changes from master
2. Submit a pull request with your new tests

we can close this issue, which would be rad.

For now, I'm just going to leave the morph stuff as-is. There's a small chance that users will do it wrong, but that's really up to them. Putting in the switch might be, as you say, problematic. And splitting out two different launch scripts is kinda clunky.

[roblanf]

Oh cool, this is done!