Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ls missingness #2515

Merged
merged 1 commit into from
Jul 5, 2023
Merged

Ls missingness #2515

merged 1 commit into from
Jul 5, 2023

Conversation

astheeggeggs
Copy link
Contributor

Missingness added to forwards-backwards diploid LiS testing

Added missingness, by including an emission probability of 1 for all missing characters in the query sequence.

@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #2515 (979e14d) into main (93cd81f) will not change coverage.
The diff coverage is n/a.

❗ Current head 979e14d differs from pull request most recent head 4926a25. Consider uploading reports for the commit 4926a25 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2515   +/-   ##
=======================================
  Coverage   89.83%   89.83%           
=======================================
  Files          30       30           
  Lines       28624    28624           
  Branches     5590     5590           
=======================================
  Hits        25713    25713           
  Misses       1655     1655           
  Partials     1256     1256           
Flag Coverage Δ
c-tests 86.24% <ø> (ø)
lwt-tests 80.13% <ø> (ø)
python-c-tests 67.13% <ø> (ø)
python-tests 99.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93cd81f...4926a25. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks good to me, just needs a rebase and squash

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

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

LGTM, couple of nits.

@@ -246,6 +250,8 @@ def stupid_compress_dict(self):
u = tree.parent(u)
self.N[self.T_index[u]] += 1

print(self.T)
Copy link
Member

Choose a reason for hiding this comment

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

Stray print?

else:
query_is_hom = np.logical_not(query_is_het)

EQUAL_BOTH_HOM = np.logical_and(
Copy link
Member

Choose a reason for hiding this comment

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

All caps like this is usually reserved for constants (https://peps.python.org/pep-0008/#constants) is there a convention here that I'm not seeing?

@jeromekelleher
Copy link
Member

Let's update this and merge

@jeromekelleher
Copy link
Member

Can you squash the commits again please @astheeggeggs? Something has gone a bit haywire, there should only be one commit by the end. (I'm happy to do it, if you prefer)

@astheeggeggs
Copy link
Contributor Author

Yeah, would you mind? I've merged the various conflicts and have checked all the tests I wrote are passing. Seems fine, but my rebasing skills are terrible.

Added forwards backwards testing and now include missingness appropriately

added missingness to diploid LS

added some fixes for flake errors

added missingness to diploid viterbi

changed test_genotype_matching_fb.py

remove stray print

removed caps for bool EQUAL_BOTH_HOM etc

Removed caps for EQUAL_BOTH_HOM etc in Viterbi

removed unused imported function
@jeromekelleher jeromekelleher added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jul 5, 2023
@jeromekelleher
Copy link
Member

done - should merge automatically now

@mergify mergify bot merged commit 3d4fc51 into tskit-dev:main Jul 5, 2023
21 checks passed
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jul 5, 2023
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