Skip to content

Conversation

jeromekelleher
Copy link
Contributor

No description provided.

@coveralls
Copy link
Collaborator

coveralls commented May 20, 2025

Coverage Status

coverage: 98.182% (-0.01%) from 98.192%
when pulling b51c8d5 on jeromekelleher:plink-improvements
into 928965e on sgkit-dev:main.

@jeromekelleher jeromekelleher requested review from benjeffery and tomwhite and removed request for benjeffery May 21, 2025 09:55
@jeromekelleher jeromekelleher marked this pull request as ready for review May 21, 2025 09:55
@jeromekelleher
Copy link
Contributor Author

This is ready for a look now and should get us to full initial plink support. I think the definition of correctness here being equivalent to running plink's own VCF conversion through vcf2zarr makes things much easier, and it fairly clear we're doing the correct thing now.

@jeromekelleher
Copy link
Contributor Author

Ok, I think that's plink support done now. The coverage misses are incidental I think and we can ignore them.

Copy link
Contributor

@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.

Very nice, good to nail down and use plink --recode as the validation

# bed and bim files, but should share a .fam
self.prefix = str(prefix)
paths = PlinkPaths(
self.prefix + ".bed",
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nicer to have the PlinkPaths __init__ just take the prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pros and cons - I tend to try and keep these dataclasses as simple as possible for reasoning about serialising and so on.

("tests/data", 5045032),
# NOTE: removed this after additional plink files broke it in #390.
# It's not worth the trouble - see #330
# ("tests/data", 5053610),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, glad you removed this. Was driving me potty.

@jeromekelleher jeromekelleher enabled auto-merge May 21, 2025 12:54
Copy link
Contributor

@tomwhite tomwhite left a comment

Choose a reason for hiding this comment

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

LGTM

@benjeffery
Copy link
Contributor

Looks like CI is complaining about a missing docs section

Fixup tests for plink

Covert some corner cases in the plink outout
Disable the top-level test as it's not working well. Not clear we want
this tbh
@jeromekelleher
Copy link
Contributor Author

Ha, whoops, forgot to add the docs files!

@jeromekelleher jeromekelleher disabled auto-merge May 21, 2025 13:23
@jeromekelleher jeromekelleher added this pull request to the merge queue May 21, 2025
@benjeffery
Copy link
Contributor

Docs look good.

Merged via the queue into sgkit-dev:main with commit b87be4c May 21, 2025
14 of 15 checks passed
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.

4 participants