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

[benchmark] fix combiner benchmarks #13956

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

ehigham
Copy link
Member

@ehigham ehigham commented Oct 31, 2023

The combiner benchmarks broke following the deletion of the experimental.vcf_combiner python package. Re-implement them in terms of the vds package.

from argparse import ArgumentParser, ArgumentDefaultsHelpFormatter

from .run import cli as run
from . import compare, create_resources, combine, summarize, visualize

def main():
def main(argv=sys.argv[1:]):
Copy link
Member Author

Choose a reason for hiding this comment

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

This change makes it easier for me to launch the benchmark suite in a debugger and change which benchmark i want to debug from within the debugger.

@ehigham
Copy link
Member Author

ehigham commented Oct 31, 2023

Hi Chris, would you mind taking a look when you get a chance? I believe you might know the most about the functionality these are exercising. Do the new implementations look ok? Are they benchmarking equivalent functionality?

Copy link
Collaborator

@chrisvittal chrisvittal left a comment

Choose a reason for hiding this comment

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

This is indeed what we would like to be checking.

Comment on lines 71 to 81
def full_combiner_chr22(*paths):
with TemporaryDirectory() as tmpdir:
vc_all.run_combiner(list(paths),
out_file=tmpdir,
tmp_path='/tmp',
branch_factor=16,
reference_genome='GRCh38',
overwrite=True,
use_exome_default_intervals=True)
combiner = new_combiner(
gvcf_paths=list(paths),
output_path=tmpdir,
temp_path='/tmp',
branch_factor=16,
reference_genome='GRCh38',
use_exome_default_intervals=True
)
combiner.run()
Copy link
Collaborator

@chrisvittal chrisvittal Oct 31, 2023

Choose a reason for hiding this comment

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

I believe this is now redundant. This and vds_combiner_chr22 are nearly equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! I'll remove it. Thanks.

@danking danking merged commit 5f4508f into hail-is:main Nov 2, 2023
@ehigham ehigham deleted the ehigham/fix-combiner-benchmarks branch November 2, 2023 18:19
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