Skip to content

Variant lowlevel class #2157

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 1, 2022
Merged

Variant lowlevel class #2157

merged 1 commit into from
Apr 1, 2022

Conversation

@benjeffery
Copy link
Member

@benjeffery benjeffery commented Mar 16, 2022

Part of #605

@benjeffery
Copy link
Member Author

benjeffery commented Mar 16, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2022

rebase

✅ Branch has been successfully rebased

@codecov
Copy link

codecov bot commented Mar 16, 2022 •
edited
Loading

Codecov Report

Merging #2157 (276bec7) into main (f6eaaba) will increase coverage by 0.00%.
The diff coverage is 94.59%.

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #2157    +/-   ##
========================================
  Coverage   93.29%   93.29%            
========================================
  Files          27       27            
  Lines       25928    26039   +111     
  Branches     1163     1163            
========================================
+ Hits        24189    24294   +105     
- Misses       1709     1715     +6     
  Partials       30       30
Flag Coverage Δ
c-tests 92.20% <100.00%> (+<0.01%) ⬆️
lwt-tests 89.05% <ø> (ø)
python-c-tests 72.27% <94.54%> (+0.18%) ⬆️
python-tests 98.89% <ø> (ø)

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

Impacted Files Coverage Δ
python/_tskitmodule.c 91.01% <94.54%> (+0.06%) ⬆️
c/tskit/genotypes.c 90.17% <100.00%> (+0.02%) ⬆️

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 f6eaaba...276bec7. Read the comment docs.

@benjeffery benjeffery force-pushed the python-variant branch 3 times, most recently from 25a3455 to 70dacb9 Compare March 31, 2022 10:23
@benjeffery benjeffery marked this pull request as ready for review March 31, 2022 10:24
@benjeffery
Copy link
Member Author

benjeffery commented Mar 31, 2022

@jeromekelleher This is the basics of the Variant class -restricted_copy coming in the next PR.

@benjeffery
Copy link
Member Author

benjeffery commented Mar 31, 2022

Note that the missing coverage should be fixed when the vargen tests are bought across.

@benjeffery benjeffery mentioned this pull request Mar 31, 2022
Merged
@benjeffery benjeffery changed the title Variant class - WIP Variant lowlevel class Mar 31, 2022
jeromekelleher
jeromekelleher approved these changes Mar 31, 2022
View reviewed changes
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, spotted one small gap in testing.

python/_tskitmodule.c
args, kwds, "O&", kwlist, &tsk_id_converter, &site_id)) {
goto out;
}
err = tsk_variant_decode(self->variant, site_id, 0);
Copy link
Member

@jeromekelleher jeromekelleher Mar 31, 2022

Choose a reason for hiding this comment

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

Should get test coverage on this - won't be covered by vargen as you can't specify the ID.

Easy to do, just pass in a bad site ID.

Copy link
Member Author

@benjeffery benjeffery Mar 31, 2022

Choose a reason for hiding this comment

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

Fixed with a call to site -1

python/_tskitmodule.c Outdated
if (PyArray_SetBaseObject(array, (PyObject *) self) != 0) {
goto out;
}
/* PyArray_SetBaseObject steals a reference, so we have to incref the tree
Copy link
Member

@jeromekelleher jeromekelleher Mar 31, 2022

Choose a reason for hiding this comment

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

out of date comment here, no tree

Copy link
Member Author

@benjeffery benjeffery Mar 31, 2022

Choose a reason for hiding this comment

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

Now you know where I copy pasted from 😃! Fixed,

@benjeffery
Copy link
Member Author

benjeffery commented Mar 31, 2022

@jeromekelleher fixed up!

@jeromekelleher
Copy link
Member

jeromekelleher commented Apr 1, 2022

Looks good to go once linting CI is updated?

@benjeffery
Copy link
Member Author

benjeffery commented Apr 1, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Apr 1, 2022

rebase

✅ Branch has been successfully rebased

@mergify mergify bot merged commit fecad52 into tskit-dev:main Apr 1, 2022
@benjeffery benjeffery deleted the python-variant branch April 4, 2022 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeromekelleher jeromekelleher jeromekelleher approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@benjeffery @jeromekelleher