Metafounder updates #175

RosCraddock · 2024-09-04T13:47:23Z

Completed updates for metafounder implementation (links to issue #142 ). There is more detail in the commits, but just for an overview:

Added two input parameters: alt_allele_prob_file and main_metafounder
Code to update the anterior of founders based on user-inputted alternative allele frequency.
New function for outputting the alternative allele frequency when alt_allele_prob_file is not inputted.
Minor updates to Usage.file

Please review when you can and let me know if you have any suggestions/changes @XingerTang

Main updates - Updating anterior of founders based on user-inputted alternative allele frequency. - Addition of two inputs: alt_allele_prob_file and main_metafounder -minor change to est_alt_allele_prob help description -If condition to output alternative allele frequencies when user has not inputted them. Minor updates - Reordering of Input Options - Minor corrections - Update to the alt_allele_prob.txt output for metafounders New writeOutAltAlleleProb Function - New function to save the alternative allele frequencies for each metafounder per the example output described in Usage file.

XingerTang

@RosCraddock Thank you for your work! Please check some of the comments I make.

src/tinypeel/Peeling/PeelingIO.py

src/tinypeel/tinypeel.py

Tinypeel.py -Additional code to save default maf to metafounder(s) in pedigree.AAP when alt_allele_prob not inputted. -Reorder of if statement so updateMaf called after pedigree.AAP defined. -Removal of redundant code -Addition of alt_allele_prob output. -Minor corrections PeelingUpdates.py -Update to updateMaf() for metafounders. Uses a for loop to update each metafounder. The actual calculation will be reviewed later. PeelingIO.py -Update to writeOutAltAlleleProb() to use pedigree.AAP (rather than reestimate from the anterior in peeling). Usage.rst -Addition of alt_allele_prob output. -Update to main_metafounders. -Small updates to descriptions for new output option. -Changed all in text references for files to italics for consistency. run_func_test.py -Updated the functional tests for alt_allele_prob output.

XingerTang

A minor issue, please see the comment.

I personally still tend to not introduce default alternative allele probability to other metafounders, as it would increase the number of the anterior probabilities that need to be updated while it won't make any difference before and after the update because the initialized anterior probabilities are also generated based on the 0.5 alternative allele probabilities.

src/tinypeel/tinypeel.py

XingerTang · 2024-09-06T14:31:33Z

@RosCraddock I noticed that the pre-commit check is not passed, if you haven't installed/used the pre-commit before, we can schedule a meeting for this.

Reference update

gregorgorjanc

LGTM

RosCraddock · 2024-10-08T10:15:24Z

@gregorgorjanc Apologies for all the commits, I will squash them all shortly. Once the tinyhouse pull request has been merged, this should pass the GitHub action checks. Then, I believe testing is complete for the metafounders (without estimation of the alternative allele frequency).

gregorgorjanc · 2024-10-08T12:01:24Z

No worries @RosCraddock! I have dealt with that PR. Let's see how action checks work and then move onwards. Please remind me once actions work;)

XingerTang · 2024-10-09T15:02:34Z

@RosCraddock Despite the exit code, everything is very good! The exit code is not very informative regarding which specific errors have been raised, ideally, we would want to assess the error message. I know it's not easy to implement, but maybe we can try.

RosCraddock · 2024-10-09T15:16:41Z

@XingerTang I agree; I just could not get it to work using the subprocess. The code would pick up the error but flag a subprocess error, resulting in the test failing before getting to the assertion. However, from what I could tell, the subprocess was coded correctly. I spent a lot of time trying to solve it, so I was keen to move on! But I can review it with fresh eyes in a couple of days.

XingerTang · 2024-10-09T21:09:44Z

@RosCraddock Thank you for the effort, I can see it won't be easy... Could you try capsys() or/and the with pytest.raises(<Error Type>, match = <Error Message>)? If this still doesn't work then we can skip this part.

RosCraddock · 2024-10-10T17:00:37Z

@XingerTang I have quickly tried both with no success unfortunately!

XingerTang · 2024-10-10T17:07:07Z

@XingerTang I have quickly tried both with no success unfortunately!

@RosCraddock OK, leave it there for now then. Maybe open an issue to come back later?

gregorgorjanc · 2024-10-22T08:32:15Z

@RosCraddock what is the next action with this PR?

RosCraddock · 2024-10-22T09:04:06Z

@RosCraddock what is the next action with this PR?

@gregorgorjanc I noticed another bug yesterday in Tinyhouse that I have now resolved (see pull request 18). Once that is reviewed and merged, I can update the submodule here. Then, this PR can be merged. However, the estimation of the alternative allele frequency with metafounders is not yet functional. When the code is ready, I can start a new PR for that. I am still doing some testing locally - will add to the issue later today!

gregorgorjanc

LGTM!

gregorgorjanc · 2024-10-23T06:03:56Z

@RosCraddock I had a look at the changes. Most of it looks ok. There is the unresolved question on what to do when a user skips one MF in the provided file compared to a pedigree AND when a user provides a MF in the provided file that is not in a pedigree. We could just throw errors when either of that happens and ping it back to a user!

@XingerTang any additional thoughts?

RosCraddock · 2024-10-23T08:43:46Z

@RosCraddock I had a look at the changes. Most of it looks ok. There is the unresolved question on what to do when a user skips one MF in the provided file compared to a pedigree AND when a user provides a MF in the provided file that is not in a pedigree. We could just throw errors when either of that happens and ping it back to a user!

@XingerTang any additional thoughts?

I previously added a conditional statement to assign the default where a MF is not listed in the alt allele prob file but is in the pedigree. I will add a test case for this. I did not consider the other scenario, but I think the addition would be ignored in the peeling but listed in the alt allele prob output (if requested). Again, I will add a test case for this. @gregorgorjanc

To add: I will remove any extra metafounders not used in peeling from the alt_allele_prob output.

XingerTang · 2024-10-23T09:20:52Z

All look to me :)

gregorgorjanc · 2024-10-23T09:44:19Z

@RosCraddock I like your use of the default when a MF is not provided by the user! Neat and consistent behaviour!

I am thinking what to do when user provides MF allele freq input for a non-existing MF in pedigree. Options:

throw an error (annoying)
ignore the extra/unneeded input (which will happen by default) and proceed, and when requested output the extra/unneeded input (might be confusing)
ignore the extra/unneeded input (which will happen by default) and proceed, and when requested NOT output the extra/unneeded input (clean solution I think)

So, probably best to go for 3) and add a warning printout to the screen/log?

RosCraddock · 2024-10-23T09:48:27Z

@gregorgorjanc I agree, I will go ahead with 3 with an addition of a warning to inform the user!

gregorgorjanc

LGTM! but would appreciate your thoughts as well @XingerTang !

gregorgorjanc · 2024-10-23T16:50:09Z

@RosCraddock we mentioned to "warn" about a deleted non-existent MF in pedigree. Do we have any established way we report such warnings?

RosCraddock · 2024-10-23T17:40:08Z

Apologies @gregorgorjanc, I forgot to add the warning. I have just pushed that through now. I am not sure it is an established way, but it seems that these warnings are just printed in the terminal prior to the peeling commencing.

gregorgorjanc

LGTM!

XingerTang · 2024-10-23T21:14:07Z

@RosCraddock tinyhouse.InputOutput are using warning.warn to raise the warnings, maybe we can also use it?

RosCraddock · 2024-10-24T07:49:54Z

@XingerTang Ahh, ok. I did not check what happened in tinyhouse. I will implement the warning.warn. I think there are some other parts in AlphaPeel that could be updated with that too so will create an issue as well.

gregorgorjanc · 2024-10-25T04:58:48Z

@RosCraddock thanks for using the warning!

@RosCraddock @XingerTang I think we are nearing to a merge of this PR. I started from the top and tried to check if we covered everything discussed, and it seems we did! Well done @RosCraddock! Am I missing anything left out (squash of the commits I reckon)?

I am only confused about the bit mentioned in #175 (comment) - should we discuss this here or in person and bring this PR to a merge?

RosCraddock · 2024-10-28T11:35:32Z

Sorry, it seems I have added a conflict with the submodule tinyhouse when I squashed the commits. Not sure how I can resolve this? @XingerTang @gregorgorjanc

(edited) I have checked all the files, and all are matching the last commit in tinyhouse/feat_metafounders (6e368443b186164ea889fa01de1a19c1aa5d2409). There were no conflicts before squashing commits, and I did not introduce anything else. So not sure why it is happening.

gregorgorjanc · 2024-10-28T13:56:45Z

@XingerTang if you have time I would appreciate if you can look into this - I am in grant and manuscript mode just now;)

gregorgorjanc · 2024-10-28T13:58:52Z

@RosCraddock silly question and sorry if I overlooked this, but how do you switch as a user between the current default Newton update of allele frequency for whole population (works with one MF only)and our EM-like update of allele frequency based on founder inferred genotype probabilities (with one or more MF)?

RosCraddock · 2024-10-28T14:24:44Z

@RosCraddock silly question and sorry if I overlooked this, but how do you switch as a user between the current default Newton update of allele frequency for whole population (works with one MF only)and our EM-like update of allele frequency based on founder inferred genotype probabilities (with one or more MF)?

@gregorgorjanc It's not a silly question, and you have not overlooked it! I have yet to push any code for estimating the alternative allele frequency with multiple metafounders as I was waiting for this PR to be completed. Then, I can start another PR for estimating allele frequencies. I have yet to implement the user-input side, but I intend to add another command called -update_alt_allele_prob_after_peeling (or -update_alt_allele_prob_for_metafounders) and update the documentation for multiple metafounders. This command would function with -est_alt_allele_prob, -alt_allele_prob_file, or using default allele frequencies whether there are one or more metafounders.

gregorgorjanc · 2024-10-28T15:13:20Z

@gregorgorjanc It's not a silly question, and you have not overlooked it! I have yet to push any code for estimating the alternative allele frequency with multiple metafounders as I was waiting for this PR to be completed. Then, I can start another PR for estimating allele frequencies. I have yet to implement the user-input side, but I intend to add another command called -update_alt_allele_prob_after_peeling (or -update_alt_allele_prob_for_metafounders) and update the documentation for multiple metafounders. This command would function with -est_alt_allele_prob, -alt_allele_prob_file, or using default allele frequencies whether there are one or more metafounders.

Gotcha! @RosCraddock Let's talk in the new PR on how to go about doing this from the option point of view. I think we probably want to stick only to -est_alt_allele_prob and -est_alt_allele_prob_method or something similar and whether this is done for 1 or more MF is simply a function of the data input (current Newton will not work for multiple MF - would need adaptation).

XingerTang · 2024-10-28T21:28:23Z

Sorry, it seems I have added a conflict with the submodule tinyhouse when I squashed the commits. Not sure how I can resolve this? @XingerTang @gregorgorjanc

(edited) I have checked all the files, and all are matching the last commit in tinyhouse/feat_metafounders (6e368443b186164ea889fa01de1a19c1aa5d2409). There were no conflicts before squashing commits, and I did not introduce anything else. So not sure why it is happening.

@RosCraddock I saw you committed a new reference update to the tinyhouse, and it seems up to date for me. Is your problem now resolved?

Or if your problem is with squashing, could you provide the exact git instructions you used to squash the changes?

RosCraddock · 2024-10-28T23:11:35Z

@RosCraddock I saw you committed a new reference update to the tinyhouse, and it seems up to date for me. Is your problem now resolved?

Or if your problem is with squashing, could you provide the exact git instructions you used to squash the changes?

@XingerTang I thought that may resolve it, but the conflict still remains. For the squashing, I followed the instructions in this stack overflow: https://stackoverflow.com/questions/5189560/how-do-i-squash-my-last-n-commits-together

git reset --soft HEAD~17
git commit
git push --force-with-lease

Tomorrow, I will try reversing the (two) commits, updating my branch (pull down), and then squashing the commits. I think that is where the conflict has emerged, as I may have accidentally removed the previous pull (my feat_metafounder branch is 7 commits behind the origin/feat_metafounder branch but two commits ahead).

Feat metafounder correction

RosCraddock · 2024-10-29T11:40:08Z

I think I have corrected my mistake and the conflict that occurred. I essentially reverted the squash to a new branch, squashed some commits I was able to without introducing the conflict, and then merged the new branch into the feat_metafounders branch. I am reluctant to try squashing the commits any further in case I introduce another conflict.
@gregorgorjanc @XingerTang

XingerTang · 2024-10-29T21:24:45Z

@RosCraddock Thank you for your work! It looks good to me. But for the squashing, maybe we need to find a better way to work with it and the submodule reference.

gregorgorjanc · 2024-10-30T08:40:21Z

@RosCraddock well done. I have struggled with squashing a couple of times too so let's merge this in so we can move onwards. Well done @RosCraddock on leading on this work!

Merge pull request AlphaGenes#175 from RosCraddock/feat_metafounders

XingerTang suggested changes Sep 5, 2024

View reviewed changes

RosCraddock commented Sep 5, 2024

View reviewed changes

src/tinypeel/tinypeel.py Outdated Show resolved Hide resolved

XingerTang suggested changes Sep 6, 2024

View reviewed changes

src/tinypeel/tinypeel.py Show resolved Hide resolved

Update and pre-commit corrections

c5af5b5

RosCraddock force-pushed the feat_metafounders branch from d9ba6f9 to c5af5b5 Compare September 10, 2024 14:58

Merge pull request #3 from AlphaGenes/feat_metafounders

ba60027

Reference update

gregorgorjanc approved these changes Oct 3, 2024

View reviewed changes

gregorgorjanc approved these changes Oct 23, 2024

View reviewed changes

Metafounder Updates

b0b9ac1

RosCraddock force-pushed the feat_metafounders branch from e472235 to b0b9ac1 Compare October 28, 2024 11:29

Submodule Update

88e5ce3

RosCraddock added 2 commits October 29, 2024 11:22

Metafounder_Updates

3b118d7

Merge pull request #4 from RosCraddock/feat_metafounder_correction

666ee62

Feat metafounder correction

gregorgorjanc merged commit 253e546 into AlphaGenes:feat_metafounders Oct 30, 2024
3 checks passed

RosCraddock mentioned this pull request Oct 30, 2024

Implement alt_allele_prob option #142

Open

RosCraddock added a commit to RosCraddock/AlphaPeel that referenced this pull request Nov 12, 2024

Merge pull request #5 from AlphaGenes/feat_metafounders

689fd29

Merge pull request AlphaGenes#175 from RosCraddock/feat_metafounders

Metafounder updates #175

Metafounder updates #175

Conversation

RosCraddock commented Sep 4, 2024

XingerTang left a comment

Choose a reason for hiding this comment

XingerTang left a comment

Choose a reason for hiding this comment

XingerTang commented Sep 6, 2024

gregorgorjanc left a comment

Choose a reason for hiding this comment

RosCraddock commented Oct 8, 2024

gregorgorjanc commented Oct 8, 2024

XingerTang commented Oct 9, 2024

RosCraddock commented Oct 9, 2024

XingerTang commented Oct 9, 2024

RosCraddock commented Oct 10, 2024

XingerTang commented Oct 10, 2024

gregorgorjanc commented Oct 22, 2024

RosCraddock commented Oct 22, 2024

gregorgorjanc left a comment

Choose a reason for hiding this comment

gregorgorjanc commented Oct 23, 2024

RosCraddock commented Oct 23, 2024 • edited Loading

XingerTang commented Oct 23, 2024

gregorgorjanc commented Oct 23, 2024

RosCraddock commented Oct 23, 2024

gregorgorjanc left a comment

Choose a reason for hiding this comment

gregorgorjanc commented Oct 23, 2024

RosCraddock commented Oct 23, 2024 • edited Loading

gregorgorjanc left a comment

Choose a reason for hiding this comment

XingerTang commented Oct 23, 2024

RosCraddock commented Oct 24, 2024

gregorgorjanc commented Oct 25, 2024

RosCraddock commented Oct 28, 2024 • edited Loading

gregorgorjanc commented Oct 28, 2024

gregorgorjanc commented Oct 28, 2024

RosCraddock commented Oct 28, 2024 • edited Loading

gregorgorjanc commented Oct 28, 2024

XingerTang commented Oct 28, 2024 • edited Loading

RosCraddock commented Oct 28, 2024 • edited Loading

RosCraddock commented Oct 29, 2024

XingerTang commented Oct 29, 2024

gregorgorjanc commented Oct 30, 2024

RosCraddock commented Oct 23, 2024 •

edited

Loading

RosCraddock commented Oct 23, 2024 •

edited

Loading

RosCraddock commented Oct 28, 2024 •

edited

Loading

RosCraddock commented Oct 28, 2024 •

edited

Loading

XingerTang commented Oct 28, 2024 •

edited

Loading

RosCraddock commented Oct 28, 2024 •

edited

Loading