Isobaric Quant FileInfo and PlexAnnotation to achieve something analogous to SpectraFileInfo#990
Isobaric Quant FileInfo and PlexAnnotation to achieve something analogous to SpectraFileInfo#990trishorts wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces data model classes for isobaric quantification workflows, creating an analogous structure to the existing SpectraFileInfo class. The implementation provides a way to represent isobaric quantification input files along with their channel-specific metadata annotations.
Key Changes:
- Added
IsobaricQuantFileInfoclass to encapsulate file metadata including path, plex type, fraction, technical replicate, and channel annotations - Added
IsobaricQuantPlexAnnotationclass to represent individual reporter channel metadata (tag, sample name, condition, biological replicate) - Added unit tests to verify basic property assignment for both classes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| mzLib/MassSpectrometry/IsobaricQuantFileInfo.cs | Implements two new classes for representing isobaric quantification file information and channel annotations with immutable core properties |
| mzLib/Test/Omics/IsobaricQuantFileAndSampleTests.cs | Adds basic unit test to validate constructor and property assignment for the new classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <returns>Hash code.</returns> | ||
| public override int GetHashCode() | ||
| { | ||
| return FullFilePathWithExtension.GetHashCode(); |
There was a problem hiding this comment.
The GetHashCode() method will throw a NullReferenceException if FullFilePathWithExtension is null (which is possible given the constructor doesn't normalize null values).
For consistency with SpectraFileInfo, use:
return StringComparer.Ordinal.GetHashCode(FullFilePathWithExtension ?? string.Empty);Or alternatively, if the constructor is fixed to normalize nulls to empty strings (as suggested in another comment), this would be acceptable as-is.
| return FullFilePathWithExtension.GetHashCode(); | |
| return StringComparer.Ordinal.GetHashCode(FullFilePathWithExtension ?? string.Empty); |
| /// <returns>File name portion of <see cref="FullFilePathWithExtension"/>.</returns> | ||
| public override string ToString() | ||
| { | ||
| return Path.GetFileName(FullFilePathWithExtension); |
There was a problem hiding this comment.
The ToString() method will throw a NullReferenceException if FullFilePathWithExtension is null (which is possible given the constructor doesn't normalize null values). Either fix the constructor to normalize nulls, or add null handling here:
return Path.GetFileName(FullFilePathWithExtension ?? string.Empty);| return Path.GetFileName(FullFilePathWithExtension); | |
| return Path.GetFileName(FullFilePathWithExtension ?? string.Empty); |
| if (base.Equals(obj)) | ||
| { | ||
| return ((IsobaricQuantFileInfo)obj).FullFilePathWithExtension.Equals(FullFilePathWithExtension); | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
The Equals method implementation is incorrect. It calls base.Equals(obj) first, which will only return true if both references point to the same object. This means the custom equality check on line 69 will never execute for different instances, even if they have the same file path.
The correct pattern (as seen in SpectraFileInfo) should be:
if (obj is not IsobaricQuantFileInfo other)
{
return false;
}
return FullFilePathWithExtension.Equals(other.FullFilePathWithExtension);Alternatively, use:
return obj is IsobaricQuantFileInfo other && FullFilePathWithExtension.Equals(other.FullFilePathWithExtension);| if (base.Equals(obj)) | |
| { | |
| return ((IsobaricQuantFileInfo)obj).FullFilePathWithExtension.Equals(FullFilePathWithExtension); | |
| } | |
| return false; | |
| return obj is IsobaricQuantFileInfo other && FullFilePathWithExtension.Equals(other.FullFilePathWithExtension); |
| public void TestIsobaricQuantFileAndPlexAnnotationCreation() | ||
| { | ||
| // Create annotations for the plex | ||
| var annotations = new List<IsobaricQuantPlexAnnotation> | ||
| { | ||
| new IsobaricQuantPlexAnnotation | ||
| { | ||
| Tag = "126", | ||
| SampleName = "Sample1", | ||
| Condition = "Control", | ||
| BiologicalReplicate = 1 | ||
| } | ||
| }; | ||
|
|
||
| // Create an instance of IsobaricQuantFileInfo | ||
| var fileInfo = new IsobaricQuantFileInfo( | ||
| fullFilePathWithExtension: "SampleQuantFile.raw", | ||
| plex: "TMT10", | ||
| fraction: 1, | ||
| technicalReplicate: 1, | ||
| annotations: annotations); | ||
|
|
||
| // Assert that the properties are set correctly | ||
| Assert.That(fileInfo.FullFilePathWithExtension, Is.EqualTo("SampleQuantFile.raw")); | ||
| Assert.That(fileInfo.Plex, Is.EqualTo("TMT10")); | ||
| Assert.That(fileInfo.Fraction, Is.EqualTo(1)); | ||
| Assert.That(fileInfo.TechnicalReplicate, Is.EqualTo(1)); | ||
| Assert.That(fileInfo.Annotations, Is.Not.Null); | ||
| Assert.That(fileInfo.Annotations.Count, Is.EqualTo(1)); | ||
| Assert.That(fileInfo.Annotations[0].Tag, Is.EqualTo("126")); | ||
| Assert.That(fileInfo.Annotations[0].SampleName, Is.EqualTo("Sample1")); | ||
| Assert.That(fileInfo.Annotations[0].Condition, Is.EqualTo("Control")); | ||
| Assert.That(fileInfo.Annotations[0].BiologicalReplicate, Is.EqualTo(1)); | ||
| } |
There was a problem hiding this comment.
The test only validates basic property assignment but doesn't test the Equals(), GetHashCode(), or ToString() methods implemented in IsobaricQuantFileInfo. Consider adding test cases to verify:
- Two
IsobaricQuantFileInfoobjects with the same file path are equal - Objects with different file paths are not equal
GetHashCode()returns the same value for equal objectsToString()returns the expected file name
This is important given that the Equals() implementation has a bug (see separate comment).
| /// <param name="annotations">Ordered list of channel annotations for the file's plex (may be empty).</param> | ||
| public IsobaricQuantFileInfo(string fullFilePathWithExtension, string plex, int fraction, int technicalReplicate, IReadOnlyList<IsobaricQuantPlexAnnotation> annotations) | ||
| { | ||
| FullFilePathWithExtension = fullFilePathWithExtension; |
There was a problem hiding this comment.
The constructor doesn't validate or normalize the fullFilePathWithExtension parameter before assigning it. Unlike SpectraFileInfo which normalizes null to string.Empty, this implementation allows null to be stored directly, which could cause NullReferenceException when calling GetHashCode() or ToString() methods.
Consider adding null-coalescing:
FullFilePathWithExtension = fullFilePathWithExtension ?? string.Empty;| FullFilePathWithExtension = fullFilePathWithExtension; | |
| FullFilePathWithExtension = fullFilePathWithExtension ?? string.Empty; |
| { | ||
| if (base.Equals(obj)) | ||
| { | ||
| return ((IsobaricQuantFileInfo)obj).FullFilePathWithExtension.Equals(FullFilePathWithExtension); |
There was a problem hiding this comment.
The string comparison should explicitly specify StringComparison.Ordinal for consistency and clarity, matching the pattern used in SpectraFileInfo. This ensures case-sensitive, culture-invariant comparison of file paths.
Consider:
return string.Equals(FullFilePathWithExtension, other.FullFilePathWithExtension, StringComparison.Ordinal);| return ((IsobaricQuantFileInfo)obj).FullFilePathWithExtension.Equals(FullFilePathWithExtension); | |
| return ((IsobaricQuantFileInfo)obj).FullFilePathWithExtension.Equals(FullFilePathWithExtension, StringComparison.Ordinal); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #990 +/- ##
=======================================
Coverage 81.08% 81.09%
=======================================
Files 271 272 +1
Lines 39154 39209 +55
Branches 4296 4306 +10
=======================================
+ Hits 31749 31795 +46
- Misses 6652 6656 +4
- Partials 753 758 +5
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Test] | ||
| public void TwoObjects_WithSameFilePath_AreEqual() | ||
| { | ||
| var a = new IsobaricQuantFileInfo("path/to/file.raw", "TMT10", 1, 1, new List<IsobaricQuantPlexAnnotation>()); | ||
| var b = new IsobaricQuantFileInfo("path/to/file.raw", "TMT11", 2, 2, new List<IsobaricQuantPlexAnnotation>()); | ||
|
|
||
| Assert.That(a.Equals(b), Is.True, "Instances with identical FullFilePathWithExtension should be equal."); | ||
| Assert.That(b.Equals(a), Is.True); | ||
| } |
There was a problem hiding this comment.
The test TwoObjects_WithSameFilePath_AreEqual asserts that objects with the same file path but different plex, fraction, and technical replicate values are equal. While this tests the current equality implementation, it doesn't test that objects with all identical properties are equal. Consider adding a test case that verifies two objects constructed with identical parameters are equal, to ensure the equality contract is complete.
| [Test] | ||
| public void TestIsobaricQuantFileAndPlexAnnotationCreation() | ||
| { | ||
| // Create annotations for the plex | ||
| var annotations = new List<IsobaricQuantPlexAnnotation> | ||
| { | ||
| new IsobaricQuantPlexAnnotation | ||
| { | ||
| Tag = "126", | ||
| SampleName = "Sample1", | ||
| Condition = "Control", | ||
| BiologicalReplicate = 1 | ||
| } | ||
| }; | ||
|
|
||
| // Create an instance of IsobaricQuantFileInfo | ||
| var fileInfo = new IsobaricQuantFileInfo( | ||
| fullFilePathWithExtension: "SampleQuantFile.raw", | ||
| plex: "TMT10", | ||
| fraction: 1, | ||
| technicalReplicate: 1, | ||
| annotations: annotations); | ||
|
|
||
| // Assert that the properties are set correctly | ||
| Assert.That(fileInfo.FullFilePathWithExtension, Is.EqualTo("SampleQuantFile.raw")); | ||
| Assert.That(fileInfo.Plex, Is.EqualTo("TMT10")); | ||
| Assert.That(fileInfo.Fraction, Is.EqualTo(1)); | ||
| Assert.That(fileInfo.TechnicalReplicate, Is.EqualTo(1)); | ||
| Assert.That(fileInfo.Annotations, Is.Not.Null); | ||
| Assert.That(fileInfo.Annotations.Count, Is.EqualTo(1)); | ||
| Assert.That(fileInfo.Annotations[0].Tag, Is.EqualTo("126")); | ||
| Assert.That(fileInfo.Annotations[0].SampleName, Is.EqualTo("Sample1")); | ||
| Assert.That(fileInfo.Annotations[0].Condition, Is.EqualTo("Control")); | ||
| Assert.That(fileInfo.Annotations[0].BiologicalReplicate, Is.EqualTo(1)); | ||
| } |
There was a problem hiding this comment.
The test suite is missing coverage for edge cases and validation scenarios. Consider adding tests for: null handling in constructor parameters (already documented to normalize nulls), empty string paths, equality with null objects, path normalization behavior (e.g., forward vs. backward slashes), and validation that Annotations is never null even when passed null in the constructor.
| [Test] | ||
| public void ToString_ReturnsFileNamePortion() | ||
| { | ||
| var filePath = @"C:\data\experiments\SampleQuantFile.raw"; | ||
| var fi = new IsobaricQuantFileInfo(filePath, "TMT10", 1, 1, new List<IsobaricQuantPlexAnnotation>()); | ||
|
|
||
| Assert.That(fi.ToString(), Is.EqualTo("SampleQuantFile.raw")); | ||
| } |
There was a problem hiding this comment.
The ToString test only verifies behavior with a Windows-style path. Consider adding test cases for Unix-style paths (forward slashes) and edge cases like paths ending with a slash, paths with just a filename (no directory), or empty paths to ensure cross-platform reliability.
| /// Equality compares the canonical file path. Two <see cref="IsobaricQuantFileInfo"/> | ||
| /// instances are considered equal when their <see cref="FullFilePathWithExtension"/> | ||
| /// strings are equal (ordinal comparison). |
There was a problem hiding this comment.
[nitpick] The documentation states "Equality compares the canonical file path" but the implementation doesn't actually canonicalize the path (e.g., resolve relative paths, normalize separators, or resolve symlinks). Consider updating the documentation to accurately reflect that it performs a direct string comparison of FullFilePathWithExtension, or implement actual path canonicalization if that's the intended behavior.
| /// Equality compares the canonical file path. Two <see cref="IsobaricQuantFileInfo"/> | |
| /// instances are considered equal when their <see cref="FullFilePathWithExtension"/> | |
| /// strings are equal (ordinal comparison). | |
| /// Equality compares the file path string directly (ordinal comparison). Two <see cref="IsobaricQuantFileInfo"/> | |
| /// instances are considered equal when their <see cref="FullFilePathWithExtension"/> | |
| /// strings are equal (ordinal comparison). No path canonicalization is performed. |
| public string Tag { get; set; } = ""; | ||
|
|
||
| /// <summary> | ||
| /// Human-readable sample name associated with this reporter channel. | ||
| /// </summary> | ||
| public string SampleName { get; set; } = ""; | ||
|
|
||
| /// <summary> | ||
| /// Experimental condition or group for this reporter channel (for example "Control" or "Treatment"). | ||
| /// </summary> | ||
| public string Condition { get; set; } = ""; | ||
|
|
||
| /// <summary> | ||
| /// Biological replicate index for this channel (1-based). Zero or negative indicates 'not set'. | ||
| /// </summary> | ||
| public int BiologicalReplicate { get; set; } |
There was a problem hiding this comment.
The IsobaricQuantFileInfo class is documented as "immutable" (line 12), but IsobaricQuantPlexAnnotation has mutable properties (setters on lines 102, 107, 112, 117). This breaks the immutability contract since Annotations is exposed as IReadOnlyList, but the annotation objects themselves can be modified after construction. Consider making IsobaricQuantPlexAnnotation properties init-only or readonly to maintain true immutability.
| public string Tag { get; set; } = ""; | |
| /// <summary> | |
| /// Human-readable sample name associated with this reporter channel. | |
| /// </summary> | |
| public string SampleName { get; set; } = ""; | |
| /// <summary> | |
| /// Experimental condition or group for this reporter channel (for example "Control" or "Treatment"). | |
| /// </summary> | |
| public string Condition { get; set; } = ""; | |
| /// <summary> | |
| /// Biological replicate index for this channel (1-based). Zero or negative indicates 'not set'. | |
| /// </summary> | |
| public int BiologicalReplicate { get; set; } | |
| public string Tag { get; init; } = ""; | |
| /// <summary> | |
| /// Human-readable sample name associated with this reporter channel. | |
| /// </summary> | |
| public string SampleName { get; init; } = ""; | |
| /// <summary> | |
| /// Experimental condition or group for this reporter channel (for example "Control" or "Treatment"). | |
| /// </summary> | |
| public string Condition { get; init; } = ""; | |
| /// <summary> | |
| /// Biological replicate index for this channel (1-based). Zero or negative indicates 'not set'. | |
| /// </summary> | |
| public int BiologicalReplicate { get; init; } |
| public override bool Equals(object obj) | ||
| { | ||
| return obj is IsobaricQuantFileInfo other && FullFilePathWithExtension.Equals(other.FullFilePathWithExtension); | ||
| } |
There was a problem hiding this comment.
The Equals method should be paired with an implementation of IEquatable<IsobaricQuantFileInfo> for better performance and type safety. This is a standard pattern for value objects that override Equals. Additionally, consider overriding the == and != operators for consistency with value object semantics.
This pull request introduces a new value object,
IsobaricQuantFileInfo, to encapsulate metadata about isobaric quantification files and their channel annotations. It also adds a supporting classIsobaricQuantPlexAnnotationand comprehensive unit tests to validate construction, equality, hash code, and edge-case behavior. These changes improve the handling and comparison of isobaric quantification file metadata, ensuring robust dictionary key usage and result table representation.New value object for file metadata and annotations:
IsobaricQuantFileInfoclass inmzLib/MassSpectrometry/IsobaricQuantFileInfo.csto represent a single isobaric quantification input file, including its canonical path, plex identifier, fraction, technical replicate, and an ordered set of per-channel annotations. Paths are normalized for robust identity comparison.IsobaricQuantPlexAnnotationclass to describe individual reporter/tag metadata within a plex, including tag, sample name, condition, and biological replicate index.Testing and validation:
IsobaricQuantFileAndSampleTestsinmzLib/Test/Omics/IsobaricQuantFileAndSampleTests.csto verify construction, equality, hash code stability, edge cases (null/empty/filename-only paths), and correct ToString behavior.Robustness and edge case handling:
IsobaricQuantFileInfo, to encapsulate metadata for isobaric quantification input files, along with its associated per-channel annotation class. Additionally, a new test suite validates the correct behavior and equality logic of these classes.New isobaric quantification file metadata classes:
IsobaricQuantFileInfoclass inmzLib/MassSpectrometry/IsobaricQuantFileInfo.csto represent a single quantification file, including file path, plex identifier, fraction, technical replicate, and an ordered, read-only list of per-channel annotations. This class is designed to be immutable and suitable for use as a dictionary key or in result tables.IsobaricQuantPlexAnnotationclass inmzLib/MassSpectrometry/IsobaricQuantFileInfo.csto describe a single reporter/tag within a plex, mapping reporter channels to sample metadata such as tag label, sample name, condition, and biological replicate.Unit tests for new value objects:
IsobaricQuantFileAndSampleTestsinmzLib/Test/Omics/IsobaricQuantFileAndSampleTests.csto verify correct construction, property assignment, equality logic, hash code stability, and string representation forIsobaricQuantFileInfoand its annotations.This pull request introduces new support for representing isobaric quantification input files and their associated channel annotations, along with corresponding unit tests to ensure correct construction and property assignment. The main changes are the addition of new data model classes for isobaric quantification and a test suite to validate their behavior.Isobaric Quantification Data Model:
IsobaricQuantFileInfoinmzLib/MassSpectrometry/IsobaricQuantFileInfo.csto encapsulate metadata about isobaric quantification files, including file path, plex type, fraction, technical replicate, and channel annotations.IsobaricQuantPlexAnnotationto represent metadata for individual reporter channels, such as tag, sample name, condition, and biological replicate.Unit Testing:
IsobaricQuantFileAndSampleTestsinmzLib/Test/Omics/IsobaricQuantFileAndSampleTests.csto verify correct instantiation and property assignment forIsobaricQuantFileInfoandIsobaricQuantPlexAnnotation.