Skip to content

Conversation

@benjeffery
Copy link
Member

Fixes #1180
WIP, needs stats tests

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.

LGTM, minor comments

@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #1233 (cd47046) into main (fadf01f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1233      +/-   ##
==========================================
- Coverage   93.71%   93.70%   -0.02%     
==========================================
  Files          26       26              
  Lines       21618    21599      -19     
  Branches      909      909              
==========================================
- Hits        20260    20239      -21     
- Misses       1319     1321       +2     
  Partials       39       39              
Flag Coverage Δ
c-tests 92.47% <ø> (-0.04%) ⬇️
lwt-tests 92.97% <ø> (ø)
python-c-tests 94.91% <100.00%> (ø)
python-tests 98.62% <100.00%> (ø)

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

Impacted Files Coverage Δ
c/tskit/core.c 97.03% <ø> (-0.03%) ⬇️
c/tskit/core.h 100.00% <ø> (ø)
c/tskit/genotypes.c 93.85% <ø> (-0.84%) ⬇️
python/tskit/vcf.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

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

@benjeffery
Copy link
Member Author

Adding silent mutations has made some tests error, looking into it

@jeromekelleher
Copy link
Member

Good, that's a sign it's working! Let me know if you have any questions/would like me to take a look at anything.

@benjeffery
Copy link
Member Author

Ah, the issue is that a site with only silent mutations has a blank vcf ALT column. I guess it should be ..

@benjeffery
Copy link
Member Author

Last failure is due to this change exposing #1225 to the test suite. Will fix that in seperate PR and merge before this one.

@petrelharp
Copy link
Contributor

Oh dear, this means I should finish up #1226

@jeromekelleher
Copy link
Member

What's the status here @benjeffery? Should we skip some tests here in this PR so we can merge it, then rebase #1226 and fix there?

@benjeffery
Copy link
Member Author

This only needs #1225, which I'm working on. Should be done in soon, so will just wait and rebase.

@benjeffery benjeffery force-pushed the silent-mutations branch 2 times, most recently from e5e6a10 to 431bf28 Compare March 9, 2021 11:34
@benjeffery benjeffery marked this pull request as ready for review March 9, 2021 11:34
@benjeffery
Copy link
Member Author

#1225 is getting more complicated than I realised so have added a skip here.

pos = self.transformed_positions[variant.index]
ref = variant.alleles[0]
alt = ",".join(variant.alleles[1:])
alt = ",".join(variant.alleles[1:]) if len(variant.alleles) > 1 else "."
Copy link
Member

Choose a reason for hiding this comment

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

Can confirm this is the correct behaviour.

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.

LGTM

@mergify mergify bot merged commit 2434cb6 into tskit-dev:main Mar 12, 2021
@benjeffery benjeffery deleted the silent-mutations branch March 14, 2021 00:59
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.

Support silent mutations

3 participants