Skip to content

Argument assay.type.ref was not taken into account - #306

Merged
LTLA merged 4 commits into
SingleR-inc:masterfrom
julien-roux:assay.type.ref_arg_fix
Sep 10, 2026
Merged

LTLA merged 4 commits into
SingleR-inc:masterfrom
julien-roux:assay.type.ref_arg_fix

Conversation

@julien-roux

Copy link
Copy Markdown
Contributor

This caused SingleR to fail if the normalized assay was not called logcounts in the reference dataset.

I simply added it in the trainSingleR() call

Copilot AI lite review requested due to automatic review settings September 8, 2026 10:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

A regression test covering the assay.type.ref behavior should be added (and the introduced tab indentation fixed) to prevent future breakage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a bug in SingleR() where the reference assay selection (assay.type.ref) was not propagated into the internal trainSingleR() call, causing failures when the normalized assay in the reference dataset was not named "logcounts".

Changes:

  • Pass assay.type.ref into trainSingleR() via its assay.type argument.
  • Minor formatting change in the trainSingleR() argument list (indentation).
File summaries
File Description
R/SingleR.R Ensures assay.type.ref is respected when training on reference objects with non-default assay names.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread R/SingleR.R
restrict = restrict,
test.genes=rownames(test),
check.missing=check.missing.ref,
assay.type=assay.type.ref,
Comment thread R/SingleR.R Outdated
julien-roux and others added 2 commits September 8, 2026 13:24
Spaces instead of tabs

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Add test for SingleR with renamed assay types
@LTLA

LTLA commented Sep 9, 2026

Copy link
Copy Markdown
Member

Thanks for the fix. Looks mostly good, though I would use assayNames() in the tests instead of names(assays(...)).

Did Copilot invite itself in? I don't remember enabling this.

@julien-roux

julien-roux commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

I would use assayNames() in the tests instead of names(assays(...)).

Sure, should I make the change, or do you take care of this?

Did Copilot invite itself in?

Yes 😂

@LTLA

LTLA commented Sep 9, 2026

Copy link
Copy Markdown
Member

I'll take care of it, I need to do a little administrivia anyway (e.g., NEWS, DESCRIPTION).

Also need to figure out how to kick copilot out of this repo. Its involvement here is pretty innocuous but you never know when it'll slip in some proprietary code.

@LTLA LTLA self-assigned this Sep 10, 2026
@LTLA
LTLA self-requested a review September 10, 2026 04:25
@LTLA
LTLA merged commit 53edadf into SingleR-inc:master Sep 10, 2026
1 check 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