Skip to content

Conversation

@alkaZeltser
Copy link
Collaborator

Found a bug caused by a case when a variant is missing in one but not all samples and is also in a multiallelic site. Code was not able to handle missing values. Fixed with an extra conditional + added unit test case.

  • I have read the code review guidelines and the code review best practice on GitHub check-list.

  • The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)-[brief_description_of_branch].

  • I have set up or verified the branch protection rule following the github standards before opening this pull request.

  • I have added the changes included in this pull request to NEWS under the next release version or unreleased, and updated the date.

  • I have updated the version number in metadata.yaml and DESCRIPTION.

  • Both R CMD build and R CMD check run successfully.

Testing Results

All unit tests pass

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.

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

Files not reviewed (3)
  • R/handle-multiallelic-sites.R: Language not supported
  • tests/testthat/generate-test-data.R: Language not supported
  • tests/testthat/test-pgs-application.R: Language not supported
Comments suppressed due to low confidence (1)

NEWS.md:6

  • [nitpick] Consider adding a note in this entry to mention that a corresponding unit test was added, which can help clarify the verification of the fix.
* Fixed bug caused by the case of a sample-specific missing variant at a multiallelic site

@alkaZeltser
Copy link
Collaborator Author

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

Files not reviewed (3)
Comments suppressed due to low confidence (1)

LOL Copilot does not read R yet.

Copy link

@rachelmadang rachelmadang left a comment

Choose a reason for hiding this comment

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

LGTM!

@alkaZeltser alkaZeltser merged commit 17db16f into main Apr 3, 2025
6 checks passed
@alkaZeltser alkaZeltser deleted the nzeltser-fix-missing-multiallelic-bug branch April 3, 2025 21:49
@rachelmadang
Copy link

Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (3)
Comments suppressed due to low confidence (1)

LOL Copilot does not read R yet.

Did you request a review from Copilot?

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