Skip to content

Conversation

@laggycomputer
Copy link
Collaborator

  • test: naive theta pi
  • feat: naive watterson theta


pub fn from_vcf<R: BufRead>(
mut reader: noodles::vcf::io::Reader<R>,
ploidy: usize,
Copy link
Member

Choose a reason for hiding this comment

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

Are we unable to determine the ploidy from the VCF file itself? What happens if we get the value wrong during input?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like the VCF standard never defines a place where ploidy is specified and mixed ploidy is supported as well. Not sure we can do anything.


use crate::model::{GenomeCollection, Site};

trait NaiveGlobalStatistic: Default {
Copy link
Member

Choose a reason for hiding this comment

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

I would argue to drop these traits. I think we want the naive versions of each stat to be defined somewhere (preferably where they cannot access fields of the GenomeCollection), and have a definition like:

pub fn diversity_from_sites(iter: Iterator<Item=&Site>) -> f64 {todo!()}
pub fn diversity_from_samples(iter: Iterator<Item=Sample>) -> f64 {todo!()}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GenomeCollecction.data is private so implementors already can't access it. We can redefine it in terms of iterators though, yes.

Copy link
Member

Choose a reason for hiding this comment

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

GenomeCollecction.data is private so implementors already can't access it. We can redefine it in terms of iterators though, yes.

But you have the naive GlobalPi in the same source file, which brings up the potential to access internal fields.

@molpopgen
Copy link
Member

One other comment: the conventional commit syntax for this pr should be test: rather than feat: when it comes time to merge. Only tests are affected and not the public API.

@laggycomputer laggycomputer changed the title feat: tests based on naive stat impls test: test by comparison to naive impls Apr 14, 2025
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