Fix(Spectronaut): Enable annotation to be added to input#18
Conversation
* Added annotation = NULL parameter to bigSpectronauttoMSstatsFormat (positional arg #2, mirroring bigDIANNtoMSstatsFormat from #16). * When supplied, the converter merges the annotation onto the output via MSstatsAddAnnotationBig, overriding any Condition / BioReplicate columns that came from R.Condition / R.Replicate. * Required for paired designs and other experimental layouts that Spectronaut's own annotation cannot express. * Added override test under tests/testthat/test-converters.R. See MSstats-ai/todos/active/TODO-MSBig-20260526_bigspectronaut_annotation_param.md Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds an optional ChangesAnnotation Parameter for Spectronaut Converter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
* Slotted annotation = NULL just before connection = NULL instead of at position #2, so the pre-existing positional signature (input_file, output_file_name, backend, intensity, ...) keeps working for any external positional callers. * This intentionally diverges from bigDIANNtoMSstatsFormat (#16), which puts annotation at position #2. Backward compatibility was prioritized for the Spectronaut converter because it had a longer pre-annotation life. DIANN can be re-flowed separately if consistency is needed later. * Restored the simpler positional example call (no longer needs named-arg workaround that the position-#2 signature forced). See MSstats-ai/todos/active/TODO-MSBig-20260526_bigspectronaut_annotation_param.md Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/testthat/test-converters.R (1)
134-141: ⚡ Quick winAssert persisted output also reflects annotation override
This test checks the returned object, but not the rewritten
output_file_nameartifact. Since Lines 195-196 introduce persistence behavior, add a reopen/assert step to lock that contract.Small test extension
expect_false(any(result$Condition == "FROM_SPECTRONAUT")) expect_false(any(result$BioReplicate == 999)) + + persisted <- dplyr::collect(arrow::open_dataset(output_file, format = "csv")) + persisted <- persisted[order(persisted$Run), ] + expect_equal(persisted$Condition, c("ctrl", "treat")) + expect_equal(persisted$BioReplicate, c(7L, 8L))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/testthat/test-converters.R` around lines 134 - 141, The test currently asserts the in-memory result but not the persisted artifact; after the existing checks (using result and output_file), reopen the written artifact at output_file into a new variable (e.g., persisted_result) and repeat the same assertions: expect_equal on Condition and BioReplicate and expect_false for "FROM_SPECTRONAUT" and 999 to ensure the persisted output reflects the annotation override introduced by the code that writes output_file_name; use the same column names (Condition, BioReplicate) and then keep the existing cleanup unmodified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@R/converters.R`:
- Around line 157-160: The function bigSpectronauttoMSstatsFormat changed its
parameter order and now breaks positional callers; restore compatibility by
either moving the annotation parameter after backend in the function signature
or adding a small shim at the start of bigSpectronauttoMSstatsFormat that
detects when callers passed the old positional form (i.e., when backend is
missing but annotation contains a backend-like value) and swaps arguments
accordingly: check if missing(backend) && !is.null(annotation) && annotation
%in% c(...) or matches the expected backend type/values, then assign backend <-
annotation; annotation <- NULL (or shift the third argument into backend and
fourth into annotation if present); update any internal references to use the
corrected variables.
- Around line 192-197: The current code unlinks output_file_name before calling
arrow::write_dataset (when backend == "arrow"), making overwrites non-atomic;
change the logic in the block that calls MSstatsAddAnnotationBig and
arrow::write_dataset so you write the Arrow dataset to a temporary path (e.g.,
output_file_name_tmp or tempdir()/basename(...)), then after write_dataset
succeeds replace the original output atomically by renaming/moving the temp to
output_file_name (using file.rename or a safe atomic swap), and only unlink the
original if needed on failure; ensure you still handle recursive and force
semantics and keep MSstatsAddAnnotationBig and backend checks intact.
---
Nitpick comments:
In `@tests/testthat/test-converters.R`:
- Around line 134-141: The test currently asserts the in-memory result but not
the persisted artifact; after the existing checks (using result and
output_file), reopen the written artifact at output_file into a new variable
(e.g., persisted_result) and repeat the same assertions: expect_equal on
Condition and BioReplicate and expect_false for "FROM_SPECTRONAUT" and 999 to
ensure the persisted output reflects the annotation override introduced by the
code that writes output_file_name; use the same column names (Condition,
BioReplicate) and then keep the existing cleanup unmodified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37f6107a-df08-41b4-88c4-759bd957da58
📒 Files selected for processing (5)
DESCRIPTIONR/converters.Rman/bigSpectronauttoMSstatsFormat.Rdman/dot-prefixedPath.Rdtests/testthat/test-converters.R
Motivation and Context
Spectronaut's native annotation capabilities cannot express all experimental layouts, particularly paired designs and other complex design patterns. This PR extends
bigSpectronauttoMSstatsFormatto accept an optionalannotationparameter, mirroring functionality added tobigDIANNtoMSstatsFormatin PR#16. When provided, the annotation data overrides theConditionandBioReplicatecolumns derived from Spectronaut's embedded R.Condition and R.Replicate fields, enabling users to specify custom experimental designs.Changes
Function signature updated:
bigSpectronauttoMSstatsFormatnow acceptsannotation = NULLas the second positional parameter (beforeoutput_file_name)Annotation processing logic: When
annotationis supplied, the function merges it onto the output usingMSstatsAddAnnotationBig, overriding Spectronaut-derivedCondition/BioReplicatevaluesFile system handling: For the
arrowbackend, the function unlinks the existing output file and writes the updated annotated dataset back to CSV before returningDocumentation updates:
@param annotationroxygen block documents the annotation parameter and its override behaviorMSstatsPreprocessBiginvocationRoxygen2 configuration:
DESCRIPTIONfile updated withConfig/roxygen2/version: 8.0.0andRoxygenNoteremoved, aligning package metadata with roxygen2 version 8.0.0Internal helper documentation: New roxygen2-generated documentation page added for
.prefixedPath()internal helper functionUnit Tests
tests/testthat/test-converters.R: Added test verifyingbigSpectronauttoMSstatsFormatcorrectly overridesConditionandBioReplicateusing provided annotation datareduceBigSpectronautto emit sentinel values forCondition/BioReplicateannotationparameter usingbackend = "arrow"andmax_feature_count = 1