Stdpopsim: unit tests in model review docs

[agladstein]

After https://github.com/popsim-consortium/stdpopsim/pull/513 is this part of the docs still true?

1. Developer B runs the units tests to verify the equivalence of the catalog and QC model implementations.

Maybe someone (@grahamgower @ndukler) can verify that section of the docs again and make sure everything is still true
https://stdpopsim.readthedocs.io/en/latest/development.html#demographic-model-review-process

[grahamgower]

Yep, that looks correct. Of course, the continuous intetegration will run the test too, but this is how the QC person will check the model equivalence before submitting their PR.

[agladstein]

Cool.
Related question - should we add more information in the docs for the QC'er? I think as it is right now, it wouldn't be enough detail for me to QC a model.

[ndukler]

Would an example QC implementation help?

[agladstein]

possibly. Without trying it out myself to see exactly what parts aren't clear, I think the parts that need more detail are:

remembering to register the QC model implementation

how to register QC model? (right now it says look at others for examples, but I think it'd be nice to have in the docs)

Developer B runs the units tests

How do you run the unit tests?

And, what do you do if problems arise?

[grahamgower]

possibly. Without trying it out myself to see exactly what parts aren't clear, I think the parts that need more detail are:

remembering to register the QC model implementation

how to register QC model? (right now it says look at others for examples, but I think it'd be nice to have in the docs)

The QC model code layout has an analogous structure to that of the catalog models. You implement a model, then register it below. Noah registered his QC model like this:

_species.get_demographic_model(
    "AshkSub_7G19").register_qc(GladsteinAshkSubstructure())

Developer B runs the units tests

How do you run the unit tests?

https://stdpopsim.readthedocs.io/en/latest/development.html#unit-tests

And, what do you do if problems arise?

Well, hopefully the errors give some indication of what is different between the models (Is this true? If not, that should be improved). If you can't figure it out, the next step would be:
https://stdpopsim.readthedocs.io/en/latest/development.html#arbitration

[ndukler]

No it doesn't. Usually I write a short script like this:

import stdpopsim

species = stdpopsim.get_species("HomSap")

model = species.get_demographic_model("AshkSub_7G19")
qc_model = model.qc_model

model.get_demography_debugger().print_history()
qc_model.get_demography_debugger().print_history()
model.equals(qc_model)

Then copy and paste the debugger outputs into a split screen text editor and go through it line by line

[grahamgower]

Interesting! Can you open a new issue about that @ndukler? We should certainly provide model QC failure error messages that are helpful.

[ndukler]

That would honestly be really difficult because as soon as one thing is off it may have a cascade effect throughout the entire model leading to a bunch of uninformative error messages. Equality is straightforward to check, meaningful specific outputs as to why they are not equal is less trivial (general messages are fine epoch times do not match, etc.). I find just viewing the debugger representations of the models side by side works really well.

[ndukler]

That being said we could probably have it output general error messages based on the structure of the sub-checks already in place. What do you think @jeromekelleher ?

[ndukler]

Also this functionality will likely have to be pulled into msprime as per issue #535

[jeromekelleher]

Can you give an example of an uninformative error message please @ndukler?

[ndukler]

Ah, I forgot about the verify_equal family of functions, my apologies @jeromekelleher . I agree those do output informative error messages at the right level of granularity. However, the automated qc process itself does not output those messages I think? Just states that the models are not equal? Do we want to change that so that if they are not equal it also returns the more specific errors from the verify_equal call?

[jeromekelleher]

That's a good idea, and shouldn't be too hard to do. We might need to write a custom assertModelsEqual function or something, though.

[agladstein]

So, I think someone should volunteer to update the docs with the things we've gone over above.
I've convinced my old lab mate to add their model from https://doi.org/10.1038/s41598-018-28539-y, and she should do that pretty soon. So, if we want to wait a little bit, I could do the QC on that model (or next model that comes in) and I can work on clarifying the docs for this simultaneously.

I've added https://github.com/popsim-consortium/stdpopsim/issues/552. So, we can move the discussion over there.