Skip to content

Add LOO and cross-drug test matrix generation#19

Merged
AbhirupaGhosh merged 8 commits into
mainfrom
Updated_matrix_generation
Jun 25, 2026
Merged

Add LOO and cross-drug test matrix generation#19
AbhirupaGhosh merged 8 commits into
mainfrom
Updated_matrix_generation

Conversation

@AbhirupaGhosh

Copy link
Copy Markdown
Contributor

Added functions to build leave-one-out and cross-drug test matrices from parquet files. Updated the main matrix generation function to include calls to these new functions.

Description

What kind of change(s) are included?

  • Feature (adds or updates new capabilities)
  • Bug fix (fixes an issue).
  • Enhancement (adds functionality).
  • Breaking change (these changes would cause existing functionality to not work as expected).

Checklist

Please ensure that all boxes are checked before indicating that this pull request is ready for review.

  • I have read and followed the CONTRIBUTING.md guidelines.
  • I have searched for existing content to ensure this is not a duplicate.
  • I have performed a self-review of these additions (including spelling, grammar, and related).
  • I have added comments to my code to help provide understanding.
  • I have added a test which covers the code changes found within this PR.
  • I have deleted all non-relevant text in this pull request template.
  • Reviewer assignment: Tag a relevant team member to review and approve the changes.

Added functions to build leave-one-out and cross-drug test matrices from parquet files. Updated the main matrix generation function to include calls to these new functions.
jananiravi
jananiravi previously approved these changes May 26, 2026

@jananiravi jananiravi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Due to GitHub action (gha) styler updates I missed the main changes in this pr but I think it looks good based on a quick look.

@amcim amcim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tested the functionality of the new functions. These look good, they produce the correct number of cross-drug parquets, LOO drug parquets, and there is no genome leakage in terms of training/held out drugs

There were multiple issues with CI and R CMD CHECK that I fixed. The one of note was that a query was failing, and I believe when the merge from main originally happened there was a conflict. It should be genome_drug.genome_id. The failed CI that points out this error can be looked at in previous actions, but basically I just had to correct this query.

Basically now that main will be back to passing CI, this will make reviewing the other open PRs here much more straightforward. (Particularly PR #8 )

One note I would like to make is in terms of the parsing filenames. If the filenames will be changing to remove the bug name in the prefix (such as in JRaviLab/amRviz#36) then this will need to be changed.

If CI passes, then I approve the merge

@AbhirupaGhosh AbhirupaGhosh merged commit 5d17393 into main Jun 25, 2026
7 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.

3 participants