Repository navigation
Add X chr functionality - #66
Conversation
| pointEstimates[1, i] *= e | ||
| pointEstimates[2, i] *= 1 - e | ||
| pointEstimates[3, i] *= 1 - e | ||
| elif isXChrom: |
There was a problem hiding this comment.
I am struggling since I am doing this on the fly and can’t check. @XingerTang can you have a look too?
Also, we don’t need the isXChrom block for the second haplotype below?
There was a problem hiding this comment.
"ind.haplotypes[0][i] != 9" means this individual is female. If this individual is female, the segregation probability must be pointEstimates[0, i] *= e; pointEstimates[1, i] *= e; pointEstimates[2, i] *= 1 - e; pointEstimates[3, i] *= 1 - e. Don't need to change if sirehap0 != 9 and sirehap1 != 9 and sirehap0 != sirehap1: because it is not possible for isXChrom. If you think if isXChrom: is easy to understand, I can change elif to if.
gregorgorjanc
left a comment
There was a problem hiding this comment.
@AprilYUZhang great that you are pushing this functionality! I reviewed on the go without access to computer to look into the code, so I wasn’t sure in some places if the code is correct or not or it’s just my lack of deep understanding. @XingerTang your review would be also appreciated.
|
@AprilYUZhang I left some more comments at de2b6c8#r184944694 on your latest commit |
|
Github is behaving odd lately - you might need to toggle file by clicking the > symbol to hide and the unhide file contents to see the comments |
XingerTang
left a comment
There was a problem hiding this comment.
- Pre-commit not passed
- pytest failed on my end:
(phase) xtang3@S37-50MAQ05D AlphaImpute2 % pytest tests/functional_tests
======================================== test session starts =========================================
platform darwin -- Python 3.11.11, pytest-9.0.1, pluggy-1.6.0
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/xtang3/AlphaImpute2
configfile: pyproject.toml
plugins: benchmark-5.2.3
collected 2 items
tests/functional_tests/run_func_test.py F. [100%]
============================================== FAILURES ==============================================
_______________________________________________ test_1 _______________________________________________
def test_1():
"""basic functionality test with pedigree only mode"""
os.system(
"AlphaImpute2 -genotypes tests/functional_tests/test_1/genotypes.txt -pedigree tests/functional_tests/test_1/pedigree.txt -ped_only -phase_output -seg_output -out tests/functional_tests/outputs/test_1"
)
assert os.path.exists("tests/functional_tests/outputs/test_1.genotypes")
assert os.path.exists("tests/functional_tests/outputs/test_1.haplotypes")
assert os.path.exists("tests/functional_tests/outputs/test_1.segregation")
genotypes = read_file("tests/functional_tests/outputs/test_1.genotypes")
expected_genotypes = read_file("tests/functional_tests/test_1/true_genotypes.txt")
> assert genotypes == expected_genotypes
E AssertionError: assert [['1', 1.0, 2....0, 0.0, 0.0]] == [['1', 1.0, 2....0, 0.0, 0.0]]
E
E At index 2 diff: ['3', 1.0, 2.0, 0.0, 0.0, 1.0] != ['3', 0.0, 2.0, 0.0, 0.0, 1.0]
E Use -v to get more diff
tests/functional_tests/run_func_test.py:53: AssertionError
---------------------------------------- Captured stdout call ----------------------------------------
------------------------------------------
AlphaImpute2
------------------------------------------
Version: 0.0.3
Email: alphagenes.dev@gmail.com
Website: https://github.com/AlphaGenes
------------------------------------------
Reading in AlphaGenes format: tests/functional_tests/test_1/genotypes.txt
Read in: 0.01 seconds
Pedigree Imputation Only
------------------------------------------
Number of peeling cycles: 4
Final cutoff: 0.1
Imputation cycle 1
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
Peel down: 4.86 seconds
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
Peel up: 4.89 seconds
Imputation cycle 2
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
Peel down: 0.00 seconds
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
Peel up: 0.00 seconds
Imputation cycle 3
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
Peel down: 0.00 seconds
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
Peel up: 0.00 seconds
Imputation cycle 4
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
Peel down: 0.00 seconds
False -1
[[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]
[2.8239352e-01 3.0398920e-01 3.3998200e-01 3.9997000e-01 4.9994999e-01]
[2.1760647e-01 1.9601080e-01 1.6001801e-01 1.0003000e-01 4.9999999e-05]]
False 1
[[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]
[0.25 0.25 0.25 0.25 0.25]]
Peel up: 0.00 seconds
Core peeling cycles: 9.75 seconds
Heuristic Peeling: 11.28 seconds
Writing Out Results
------------------------------------------
Write out: 0.00 seconds
Full Program Run: 11.52 seconds
|
No tests are written for this code. Convert this PR to a draft. |
|
@AprilYUZhang, please rebase the change in #68 for correct geno prob updates after cycle 2 |
|
Hi @gregorgorjanc, rough modification for X chr, now it works on the simple X chr sample. However, pytest still doesn't work. I will be back after @XingerTang updates the pytest. @XingerTang, please let me know once you finish all basic and general modifications. |
pytest doesn't work on my end, too. I have no idea why pytest isn't working, since I didn't make any modifications that might affect the function test. But the tinyhouse version is different. I used the latest main branch because it includes the updates about sex chromosomes. This is the commit that I stay:f6cd2bc45f3e81d5a2b34e093be0f14b20f4218d. I suggest prioritizing tinyhouse update on your end, and then we can figure out where the problems are together. Finally, merge your branch to this request. |
|
It must be due to code changes you have done for the autosomal case. I have spotted some odd cases and highlighted those via comments but could have missed some, I reckon. Happy bug hunting;) |
|
Just to add that you should attempt to fix this @AprilYUZhang if at all possible. @XingerTang input is of course welcome and valuable. |
I agree with you. I haven't check true value of simulation. If the problem has solved, the new simulation can be ignored. |
Regarding founder genome, back to a single runMacs call producing 2 chromosomes, then pullSegSiteHaplo for each, then importHaplo to glue on the SD locus onto the "XY" group only (chromosome "A" is untouched autosome, "XY" = X markers + SD). Everything downstream is duplicated per chromosome type. |
|
After the commit 8e77d4d, with the exact same settings as before, the runtime lasts |
|
Excellent push @XingerTang - so runtime "mystery" has now been resolved. I got lost in the process regarding the accuracies - how do they look like for A vs. X chr? Thanks! |
Currently, X chr is better than autosomal chr in all metrics and all files: The accuracy improvement is happened at: #66 (comment) |
gregorgorjanc
left a comment
There was a problem hiding this comment.
Awesome that you find solution to reduced the increased runtime - it bulks up the code, but this is the price we have to pay.
Perfect - very pleasing to see these results - they do make sense now. I am curious what was the source of the error that was puzzling @AprilYUZhang! @XingerTang is there anything else we need before we merge this in? Runtime is good (check), accuracy is good (check), documentation (done?), changelog (done?), anything else? |
There were several different things involved, but the major issue is that the XChr flag is not passed into the anterior probability calculation function in several places where it is called. I'm now trying to fix the GitHub Actions tests. Later after that, will move on to the documentation and changelog. |
|
The indexing error that kept causing access issues is fixed; here is a Codex summary of how that bug was introduced into the code:
|
Huh, interesting development of this bug! I wonder why it did not impact accuracy and interesting to see the difference between how different OS handle such a bug (Windows fail vs other somewhat tolerating it). Well done Evie and Codex;) |
|
At this point, all the changes for X chromosome imputation have been completed, with the imputation results accurate and efficient. The remaining part of this PR would be the last round of review and then squashing the commits if possible. |
gregorgorjanc
left a comment
There was a problem hiding this comment.
Looks good! It pays off we spend quite a bit of time on docs for AlphaPeel;)
|
Do we have clear small example with X chr for AlphaImpute2? Could use one from AlphaPeel (assuming we have one). Such small examples are key for users to start using a tool. |
|
@XingerTang Good job! Thank you for moving forward. |
|
I think the example mentioned by
refers to the example in the documentation, and for that the answer is no, we don't have an X chromosome example in the documentation for AlphaPeel. |
Do you mean document usage.rst? |
They are not the same; I added the pedigree file format X Chr example to the AlphaImpute2 documentation in my last commit. But here, the example probably refers to this: https://alphapeel.readthedocs.io/en/latest/get_started.html#an-example |
refactor the code to follow the order as the autosomal update PhasingObject for rec Fix anterior prob calc and more Clean up code
|
I squashed the original 56 commits into 5 commits through |
|
Great job @AprilYUZhang @XingerTang!! |

Issue #47