Skip to content

Add alternative allele probability input#15

Merged
gregorgorjanc merged 3 commits intoAlphaGenes:feat_metafoundersfrom
XingerTang:feat_metafounders
Sep 25, 2024
Merged

Add alternative allele probability input#15
gregorgorjanc merged 3 commits intoAlphaGenes:feat_metafoundersfrom
XingerTang:feat_metafounders

Conversation

@XingerTang
Copy link
Contributor

@gregorgorjanc
Could you merge this in so that we can test the code update with AlphaPeel tests

Copy link
Contributor

@RosCraddock RosCraddock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of questions/comments from me. But as always, very nicely written code!

Pedigree.py Outdated
self.individuals[idx] = self.constructor(idx, self.maxIdn)

# check if the individual is a metafounder
if idx is not None and idx[:3] != "MF_":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be reading this wrong, so do correct me if I am.
Here, we are reading in the pedigree with idx equal to an individual's id (taken from the first column of the inputted pedigree data). Thus, we should not have any idx that are equal to "MF_" since the metafounders will not have individual records. However, this if statement produces an error if the idx is not equal to "MF_". Instead, should it be is equal to, so: idx[:3] == "MF_"?

Copy link
Contributor Author

@XingerTang XingerTang Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Edited) Sorry, I just recalled the meaning of this if condition. You are right, I should simply change the unequal sign to the equal sign.

Pedigree.py Outdated
default_aap = True

for value in data_list:
idx, data = value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function reads in the user-inputted alternative allele probabilities for each metafounder. The first column is the defined metafounder (MF_x), and the remaining columns include the alternative allele probability at each locus. So, here idx is equal to MF_x. If so, would it be better to change idx to mfx to avoid confusion with the user-inputted pedigree id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I will change it in the next commit.

Copy link
Contributor Author

@XingerTang XingerTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RosCraddock Thank you for the review!

Pedigree.py Outdated
self.individuals[idx] = self.constructor(idx, self.maxIdn)

# check if the individual is a metafounder
if idx is not None and idx[:3] != "MF_":
Copy link
Contributor Author

@XingerTang XingerTang Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Edited) Sorry, I just recalled the meaning of this if condition. You are right, I should simply change the unequal sign to the equal sign.

Pedigree.py Outdated
default_aap = True

for value in data_list:
idx, data = value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I will change it in the next commit.

Copy link
Contributor

@RosCraddock RosCraddock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick fix, thank you!

self.MainMetaFounder = None
self.AAP = {}

# remove?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XingerTang about this comment (remove?) are you thinking of removing self.maf? If so, where would we be storing maf?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregorgorjanc In alphapeel the maf are stored in the peelingInfo, and this self.maf is not used. But I don't know about other software so I put a question mark.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, future work.

Copy link
Member

@gregorgorjanc gregorgorjanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@gregorgorjanc gregorgorjanc merged commit 5be64d1 into AlphaGenes:feat_metafounders Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants