Skip to content

Conversation

@gilhornung
Copy link
Collaborator

No description provided.

@gilhornung gilhornung requested review from Copilot and doron-st October 6, 2025 10:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR changes the quick fingerprinting tool to output results in CSV format instead of plain text, while adding comprehensive documentation throughout the codebase.

  • Converts output from text format to structured CSV with columns for cram_filename, sample_id, ground_truth_id, hit_fraction, and best_match
  • Adds detailed docstrings and inline comments to improve code readability and maintainability
  • Removes error checking and validation logic that was previously present in the comparison workflow

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
ugvc/pipelines/comparison/quick_fingerprinting.py Added docstring to parser function and inline comments explaining each processing step
ugvc/comparison/quick_fingerprinter.py Changed output format from text to CSV, added comprehensive documentation, and removed error validation logic

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +47 to 53
self.output_file = open(f"{self.out_dir}/quick_fingerprinting_results.csv", "w")

os.makedirs(out_dir, exist_ok=True)

# Prepare ground truth VCFs for each sample
self.ground_truths_to_check = self.prepare_ground_truth()

Copy link

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

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

The file is opened without proper resource management. Consider using a context manager or ensure the file is closed in a finally block to prevent resource leaks if an exception occurs before reaching the close() call.

Suggested change
self.output_file = open(f"{self.out_dir}/quick_fingerprinting_results.csv", "w")
os.makedirs(out_dir, exist_ok=True)
# Prepare ground truth VCFs for each sample
self.ground_truths_to_check = self.prepare_ground_truth()
self.output_file = open(f"{self.out_dir}/quick_fingerprinting_results.csv", "w")
self._file_closed = False
os.makedirs(out_dir, exist_ok=True)
# Prepare ground truth VCFs for each sample
self.ground_truths_to_check = self.prepare_ground_truth()
def __enter__(self):
return self
def __exit__(self, exc_type, exc_val, exc_tb):
self.close()
def close(self):
if hasattr(self, "output_file") and not self.output_file.closed:
self.output_file.close()
self._file_closed = True

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@ilyasoifer ilyasoifer self-requested a review October 18, 2025 20:52
@ilyasoifer
Copy link
Collaborator

@gilhornung - can you fix the test please?

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