Metafounder flexibility, alt_allele_prob_file formatting, and some name changes

[RosCraddock]

Issues

• Handling two metafounders in one individual (Issues in tinyhouse: Merged metafounder code inconsistency tinyhouse#31 and Make metafounders more flexible tinyhouse#30)
• Changing the alt_allele_prob_file input per PR Polish of the usage and TODOs to clarify and simplify options #219. This includes related tests in functional and accuracy testing.
• Issue Consolidate est/update_alt_allele_prob #221 Renaming of -est_alt_allele_prob, -update_alt_allele_prob, and -pheno_penetrance_prob. This includes renaming in testing.

Two submodule PRs:

AlphaGenes/tinyhouse#27 (already merged)
AlphaGenes/tinyhouse#32 - still to be merged. Hence, the tests will fail when using -alt_allele_prob_file command. I have completed testing locally, so all should pass once the submodule is updated after merging PR.

If you have any issues/clarifications, please let me know.

[XingerTang]

Hi @RosCraddock , thank you for your work. I haven't reviewed your changes yet, I will do that soon.
I noticed that there is a TODO in the changelog.rst (https://github.com/roscraddock/alphapeel/blob/8d7de1290d29f5223ca86e9f1a8cb9bdcf638ffb/docs/source/changelog.rst#L18) regarding renaming the pheno_penentrance_file to pheno_error_prob_file. If I remembered correctly, we later decided that we don't need this change. If that is the case, could you remove the line in this pull request? Thanks.

[RosCraddock]

Hi @RosCraddock , thank you for your work. I haven't reviewed your changes yet, I will do that soon. I noticed that there is a TODO in the changelog.rst (https://github.com/roscraddock/alphapeel/blob/8d7de1290d29f5223ca86e9f1a8cb9bdcf638ffb/docs/source/changelog.rst#L18) regarding renaming the pheno_penentrance_file to pheno_error_prob_file. If I remembered correctly, we later decided that we don't need this change. If that is the case, could you remove the line in this pull request? Thanks.

Good spot. I believe this will be updated to -pheno_penetrance_prob_file per the documentation changes. Will resolve shortly.

[XingerTang]

Hi @RosCraddock, thank you for your work. I have reviewed your code here and AlphaGenes/tinyhouse#27. It looks very good to me, and all the tests have passed on my end as well!

There is only one thing I would like to know: whether you have tested the case of missing values in the AAP input. You have the relevant code in tinyhouse https://github.com/AlphaGenes/tinyhouse/pull/32/changes#diff-a0d97e9f53f6ea79d3ccd4cb678c74cfa678a5edb8d754dcbd3dfc04c25c6d7dR1022. I don't see any reason that may lead to that part of the code failing, but it would be great if we had a test associated with it.

[RosCraddock]

Hi @RosCraddock, thank you for your work. I have reviewed your code here and AlphaGenes/tinyhouse#27. It looks very good to me, and all the tests have passed on my end as well!

There is only one thing I would like to know: whether you have tested the case of missing values in the AAP input. You have the relevant code in tinyhouse https://github.com/AlphaGenes/tinyhouse/pull/32/changes#diff-a0d97e9f53f6ea79d3ccd4cb678c74cfa678a5edb8d754dcbd3dfc04c25c6d7dR1022. I don't see any reason that may lead to that part of the code failing, but it would be great if we had a test associated with it.

Good point! I had not, and have added that now. Also added code and test to produce an error if a loci value is below 0 or above 1 (i.e 9).

In the process, I noticed some differences in error handling, where in some cases we produce an error, and in others we give a warning and/or use a default value. For example, consider these two alt_allele_prob_files in a pedigree with two metafounders:

a)
MF_1
0.6
0.7
0.6
0.6

b)
MF_1 MF_2
0.6
0.7
0.6
0.6

(a) will use a default of 0.5 for all loci of MF_2.
(b) will give an error and exit the program.

I think both approaches are correct for now, but I wonder with the new behaviour (to only output when estimated), if we should at least add a warning in the case of (a) and/or print out the output when it differs from the original input? @XingerTang @gregorgorjanc
Shall I start an issue for this?

[gregorgorjanc]

I suggest to throw an error for case b) as it can get very confusing and hard to generalise, for example, consider this case:

MF_1 MF_2 MF_3
0.2 0.3
0.8 0.6

Have we provided above allele probs for MF_1, MF_2, or for MF_3?

In the case of a) you will have, say,

MF_1 MF_3
0.2 0.3
0.8 0.6

or some other permutation, so you can then safely reason about default allele probs for the MF missing in the file.

[XingerTang]

Hi @RosCraddock, thank you for your work. I have reviewed your code here and AlphaGenes/tinyhouse#27. It looks very good to me, and all the tests have passed on my end as well!
There is only one thing I would like to know: whether you have tested the case of missing values in the AAP input. You have the relevant code in tinyhouse https://github.com/AlphaGenes/tinyhouse/pull/32/changes#diff-a0d97e9f53f6ea79d3ccd4cb678c74cfa678a5edb8d754dcbd3dfc04c25c6d7dR1022. I don't see any reason that may lead to that part of the code failing, but it would be great if we had a test associated with it.

Good point! I had not, and have added that now. Also added code and test to produce an error if a loci value is below 0 or above 1 (i.e 9).

In the process, I noticed some differences in error handling, where in some cases we produce an error, and in others we give a warning and/or use a default value. For example, consider these two alt_allele_prob_files in a pedigree with two metafounders:

a) MF_1 0.6 0.7 0.6 0.6

b) MF_1 MF_2 0.6 0.7 0.6 0.6

(a) will use a default of 0.5 for all loci of MF_2. (b) will give an error and exit the program.

I think both approaches are correct for now, but I wonder with the new behaviour (to only output when estimated), if we should at least add a warning in the case of (a) and/or print out the output when it differs from the original input? @XingerTang @gregorgorjanc Shall I start an issue for this?

I agree with @gregorgorjanc, and I found your current solution is good enough.

[XingerTang]

Hi @RosCraddock, I think everything looks great to me. Just a minor thing I commented on.
I would like to remind you that the devel branch just had a few updates, and probably a rebase is required. The changelog file has some conflicts with a previous PR. Sorry that I didn't notice that earlier, but you might need to resolve that. It would be a simple fix.

[XingerTang]

Merged with 142fd5b