Add the ability to decode the chunk layout - #15
Conversation
When array elements are lists, account for the elements in the list when reserving space in the destination vector.
…lass This will be needed to decoding null values in the chunk encoding.
When using delta encoding the start value is necessary to reconstruct the remaining values.
The name `column` is more appropriate, and the overloads reduce code bloat in other compilation units.
This is necessary for numpress decoders that need to turn an array of bytes into an array of doubles.
If the `chunk_transform` column is present then it is used instead of the `chunk_values` column, which will also be present but NULL.
The destination vector needs more space to avoid re-allocations and transformers need a way to return one extra value.
git-subtree-dir: subprojects/msnumpress git-subtree-split: 94090cf72457fe91ff709a0827ff175e53b83cd6
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe change adds layout-aware point and chunked decoding, MS-Numpress support, primary and secondary transformers, bundled language bindings, build integration, spectrum inspection output, and expanded tests. ChangesMS-Numpress decoding
Sequence Diagram(s)sequenceDiagram
participant MzPeakFile
participant ArrayIndex
participant Signals
participant Decoder
MzPeakFile->>ArrayIndex: read layout and dimension entries
Decoder->>ArrayIndex: resolve values entry
Decoder->>Signals: resolve encoded columns
Signals-->>Decoder: return Arrow arrays
Decoder->>Decoder: apply point or chunked transforms
Decoder-->>MzPeakFile: return decoded values
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
subprojects/README.md-16-19 (1)
16-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the configured Markdown code-block style.
The added fenced command blocks violate the configured Markdown rules. Use indented code blocks, or update the lint configuration if fenced blocks are intended.
subprojects/README.md#L16-L19: convert the subtree command block to the configured style.subprojects/msnumpress/README.md#L16-L18: convert the R installation command block to the configured style.subprojects/msnumpress/README.md#L23-L25: convert the conda channel command block to the configured style.subprojects/msnumpress/README.md#L28-L30: convert the conda installation command block to the configured style.subprojects/msnumpress/README.md#L67-L69: convert the R test command block to the configured style.🤖 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 `@subprojects/README.md` around lines 16 - 19, Replace the fenced command blocks with the configured indented Markdown code-block style in subprojects/README.md lines 16-19, subprojects/msnumpress/README.md lines 16-18, 23-25, 28-30, and 67-69; preserve each command and its formatting as executable text.Source: Linters/SAST tools
test/spectra_test.cpp-29-44 (1)
29-44: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake indexed collection checks fatal.
BOOST_TESTcontinues after failure. Ifspectrais empty, Line 31 can access outside the collection. Ifmzis empty, Lines 43 and 44 can access outside the collection.Use
BOOST_TEST_REQUIREfor the spectrum-count and m/z-size checks.Proposed fix
- BOOST_TEST((spectra.size() == 48)); + BOOST_TEST_REQUIRE(spectra.size() == 48); ... - BOOST_TEST(mz.size() == 13589); + BOOST_TEST_REQUIRE(mz.size() == 13589);🤖 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 `@test/spectra_test.cpp` around lines 29 - 44, In the test around spectra and m/z validation, replace the non-fatal BOOST_TEST checks for the spectrum count and mz size with BOOST_TEST_REQUIRE. Keep the existing expected values and tolerance checks unchanged so indexing spectra[0] and the first/last m/z elements only occurs after the collections are confirmed non-empty.subprojects/msnumpress/src/main/R/RMSNumpress/man/optimalLinearFixedPointMass.Rd-20-25 (1)
20-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe note states the wrong integer width.
The note says the accuracy cannot be reached "without overflowing 64 bit integers". The limit is 32 bit.
optimalLinearFixedPointMasscompares againstoptimalLinearFixedPoint, which returnsfloor(0x7FFFFFFF / maxDouble)at Line 265 ofsubprojects/msnumpress/src/main/cpp/MSNumpress.cpp.0x7FFFFFFFis the signed 32-bit maximum. The linear codec encodes residuals as signed 32-bit values.Change "64 bit integers" to "32 bit integers" in the roxygen source and regenerate this file.
🤖 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 `@subprojects/msnumpress/src/main/R/RMSNumpress/man/optimalLinearFixedPointMass.Rd` around lines 20 - 25, Update the note for optimalLinearFixedPointMass in the roxygen source to state that overflow occurs with 32-bit integers, then regenerate the corresponding RMSNumpress documentation file so the generated text matches.subprojects/msnumpress/src/main/R/RMSNumpress/man/encodeLinear.Rd-16-18 (1)
16-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe three encode functions document a byte count but return a byte vector. The roxygen
@returntags insubprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cppwere copied from the C++ signatures, which return the number of encoded bytes. The R wrappers return the encoded vector instead, as the example at Line 37 ofencodeLinear.Rdshows. Fix the@returntags at the roxygen source and regenerate all three files, because each file is marked as generated at Line 1.
subprojects/msnumpress/src/main/R/RMSNumpress/man/encodeLinear.Rd#L16-L18: change\valueto describe the returned raw vector of encoded bytes.subprojects/msnumpress/src/main/R/RMSNumpress/man/encodePic.Rd#L12-L14: change\valueto describe the returned raw vector of encoded bytes.subprojects/msnumpress/src/main/R/RMSNumpress/man/encodeSlof.Rd#L14-L16: change\valueto describe the returned raw vector of encoded bytes.🤖 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 `@subprojects/msnumpress/src/main/R/RMSNumpress/man/encodeLinear.Rd` around lines 16 - 18, Update the roxygen `@return` tags for the three encode functions in RMSNumpress.cpp to describe the returned raw vector of encoded bytes rather than a byte count, then regenerate the documentation. Apply the generated change to encodeLinear.Rd (lines 16-18), encodePic.Rd (lines 12-14), and encodeSlof.Rd (lines 14-16), with no other documentation changes.subprojects/msnumpress/src/main/cpp/MSNumpressTest.cpp-161-187 (1)
161-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe loop bound skips the last fixed-point case.
mz_errandencodedLengtheach hold 5 entries. The loop at Line 171 runsk < 4, so the1e-7case with expected length 31 never runs. The Java test insubprojects/msnumpress/src/test/java/ms/numpress/MSNumpressTest.javaiterates the full array and does cover it.💚 Proposed fix
- for (int k = 0; k < 4; k++) + for (int k = 0; k < 5; k++)🤖 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 `@subprojects/msnumpress/src/main/cpp/MSNumpressTest.cpp` around lines 161 - 187, Update the loop bound in the fixed-point accuracy test to iterate all five entries in mz_err and encodedLength, including the 1e-7 case and its expected encoded length.subprojects/msnumpress/src/main/csharp/MSNumpress.cs-365-369 (1)
365-369: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMatch the
0x0padding check indecodeLinearanddecodePic. A final0x8is a valid zero-value head. A final nonzero nibble such as0x3is malformed; C++ rejects it, but C# silently truncates the decoded data. Break only when the trailing half-byte is0x0.🤖 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 `@subprojects/msnumpress/src/main/csharp/MSNumpress.cs` around lines 365 - 369, Update the trailing half-byte validation in the affected decode loop to match decodeLinear and decodePic: when dec.pos is the final byte and dec.half is set, break only if the low nibble is 0x0. Preserve 0x8 as a valid zero-value head and reject malformed nonzero trailing nibbles such as 0x3 instead of silently truncating.subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp-103-108 (1)
103-108: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the R return-value documentation.
These Rcpp exports return raw or numeric vectors. Their Roxygen comments describe the scalar C++ byte or element counts. This gives R callers an incorrect API contract.
subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp#L103-L108: documentencodeLinearas returning an encoded raw vector.subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp#L141-L145: documentdecodeLinearas returning a decoded numeric vector.subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp#L162-L167: documentencodeSlofas returning an encoded raw vector.subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp#L194-L198: documentdecodeSlofas returning a decoded numeric vector.subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp#L214-L218: documentencodePicas returning an encoded raw vector.subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp#L235-L239: documentdecodePicas returning a decoded numeric vector.Regenerate
RcppExports.Rand the Rd files after the source documentation update.🤖 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 `@subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp` around lines 103 - 108, Update the Roxygen return documentation for encodeLinear (subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp:103-108), decodeLinear (141-145), encodeSlof (162-167), decodeSlof (194-198), encodePic (214-218), and decodePic (235-239) to describe encoded functions as returning raw vectors and decoded functions as returning numeric vectors, replacing scalar count descriptions. Regenerate RcppExports.R and the Rd files afterward.subprojects/msnumpress/src/main/python/setup.py-36-48 (1)
36-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve native source paths from
setup.py.When invoked from the repository root, both copy paths and extension sources resolve outside the Python binding directory. The suppressed
IOErrorallows the build to continue until compilation fails. Anchor the copy and extension paths toos.path.dirname(__file__), and let copy errors propagate.🤖 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 `@subprojects/msnumpress/src/main/python/setup.py` around lines 36 - 48, Update setup.py’s native source handling to resolve paths relative to os.path.dirname(__file__) for both the MSNumpress.cpp/MSNumpress.hpp copy operations and the Extension sources, so builds work from any working directory. Remove the broad IOError suppression and allow copy failures to propagate.subprojects/msnumpress/src/main/java/ms/numpress/MSNumpress.java-198-209 (1)
198-209: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe single-element fixed point uses the unsigned 32-bit range here, but the signed range in the C++ copy.
Line 199 divides by
0xFFFFFFFFl.subprojects/msnumpress/src/main/R/RMSNumpress/src/MSNumpress.cppLine 259 divides by0x7FFFFFFFlfor the same case.encodeLinearstores the first two values as signed 4-byte ints, so the Java value can produce a fixed point that overflows the signed range for one-element arrays. The Java and C++ codecs then disagree on that input.Confirm which value upstream ms-numpress carries today before you change the vendored file.
#!/bin/bash # Description: Compare the single-element fixed-point constant across every vendored numpress implementation. set -euo pipefail fd -t f -e java -e cpp -e hpp -e cs -e pyx . subprojects/msnumpress \ --exec rg -n -C 4 'dataSize == 1' {} \;🤖 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 `@subprojects/msnumpress/src/main/java/ms/numpress/MSNumpress.java` around lines 198 - 209, Confirm the upstream ms-numpress implementation’s single-element fixed-point behavior across the vendored Java and C++ codecs before editing. Then update the single-element branch in the Java fixed-point calculation to use the same signed 32-bit range as the C++ implementation and preserve agreement with encodeLinear’s signed 4-byte storage.subprojects/msnumpress/src/main/R/RMSNumpress/inst/LICENSE.md-1-19 (1)
1-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the license placeholder and state the dual licensing.
Line 17 keeps the literal placeholder
<ORGANIZATION>. The bundled C++ sources insubprojects/msnumpress/src/main/R/RMSNumpress/src/MSNumpress.cppare Apache 2.0 (Johan Teleman), while this file declares BSD 3-clause. Name the organization and record that the vendored codec sources stay under Apache 2.0. This keeps the redistribution terms of the wholesubprojects/msnumpresstree unambiguous.🤖 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 `@subprojects/msnumpress/src/main/R/RMSNumpress/inst/LICENSE.md` around lines 1 - 19, Update the BSD 3-Clause license notice in RMSNumpress to replace the literal <ORGANIZATION> placeholder with the appropriate organization name, and explicitly document that the vendored codec sources in MSNumpress.cpp remain licensed under Apache 2.0 by Johan Teleman. Preserve the existing BSD terms while clearly recording the dual licensing for the bundled sources.include/mzpeak/data/array_index.h-212-213 (1)
212-213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInitialize
layout_toLayout::Unknown.The public default constructor leaves
layout_indeterminate. A laterlayout()call can select an arbitrary decode path instead of reporting an unknown layout.Proposed fix
- Layout layout_; + Layout layout_ = Layout::Unknown;🤖 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 `@include/mzpeak/data/array_index.h` around lines 212 - 213, Initialize the `layout_` member in the default constructor of the relevant array index class to `Layout::Unknown`, ensuring subsequent `layout()` calls report an unknown layout until explicitly set.include/mzpeak/schema/group.h-123-128 (1)
123-128: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documentation to match the return type.
The comment states the function returns
true. The function returnsstd::optional<Util::Numpress::Type>. It returns the detected Numpress type, or no value when the column name gives no indication.📝 Proposed documentation fix
/** - * Return `true` if the column name indicates this is a numpress - * compressed column of `uint8_t`. + * Return the Numpress type if the column name indicates this is a + * numpress compressed column of `uint8_t`. + * + * Returns no value if the column name gives no indication. */ std::optional<Util::Numpress::Type> possibly_numpress() const;🤖 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 `@include/mzpeak/schema/group.h` around lines 123 - 128, Update the documentation for Group’s possibly_numpress() method to state that it returns the detected Util::Numpress::Type in the optional, or no value when the column name does not indicate Numpress compression, instead of claiming it returns true.include/mzpeak/util/numpress.h-72-78 (1)
72-78: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stray semicolon after the function body.
Line 78 ends with
};. The sibling functionsdecode_slof_convertat line 103 anddecode_pic_convertat line 128 end with}. The extra semicolon is an empty declaration at namespace scope. It triggers-Wextra-semiand breaks a build that uses-Werror.🔧 Proposed fix
std::shared_ptr<std::vector<double>> doubles = decode_linear(src); return cast<T>(doubles); -}; +}🤖 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 `@include/mzpeak/util/numpress.h` around lines 72 - 78, Remove the trailing semicolon after the decode_linear_convert function body, leaving the definition terminated with `}` like the sibling decode_slof_convert and decode_pic_convert functions.include/mzpeak/util/decoders.h-220-233 (1)
220-233: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
scalar_decoder_uses the outer container type, which breaks any instantiation whereCis notstd::vector<V>.
decodepassesres, of typevalue_type(std::vector<V>), toscalar_decoder_.decode.Scalar<V, C, N>::decodeacceptsC&. The call compiles only whenCis exactlystd::vector<V>.The
requires Decoders::scalar_or_container_of<C, V>constraint permits other types forC. Those instantiations fail to compile insidedecode. The inner decoder always writes intovalue_type, so declare it withvalue_type.The CRTP base is also
Helper<List<V, C>>and dropsN.List<V, C, N1>andList<V, C, N2>then share one base type.Flattenedat line 279 has the same pattern.🔧 Proposed fix for the inner decoder container type
private: - Scalar<V, C, N> scalar_decoder_; + Scalar<V, value_type, N> scalar_decoder_; };🤖 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 `@include/mzpeak/util/decoders.h` around lines 220 - 233, Update the inner decoder type in List::decode so scalar_decoder_ is instantiated with value_type rather than the outer container type C, allowing constrained C alternatives to compile while retaining the existing res decoding flow. Also update the CRTP base types for List and Flattened to include their N template parameter, ensuring different List<V, C, N> and Flattened<V, C, N> instantiations do not share a base type.
🧹 Nitpick comments (16)
test/algorithm_test.cpp (1)
18-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case with null values.
This test only validates the no-null path. It does not validate null preservation or accumulator behavior after a null entry.
Add an input with one or more Arrow nulls and assert the decoder output against the null-delta contract.
🤖 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 `@test/algorithm_test.cpp` around lines 18 - 51, Add a separate test alongside null_delta_decode_with_no_nulls that builds an Arrow Int64 input containing one or more null entries, invokes Algorithm::null_delta_decode<Type::Int64>, and verifies both output null preservation and decoded values after nulls according to the null-delta contract. Reuse the existing Arrow builder and assertion style, including coverage of accumulator behavior following a null entry.subprojects/msnumpress/src/main/python/test_pymsnumpress.py (2)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe docstring names the wrong file.
Line 4 says
setup.py. The file istest_pymsnumpress.py. This is inherited from upstream. Correct it if you intend to maintain a local fork; leave it if you plan to re-sync from upstream.🤖 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 `@subprojects/msnumpress/src/main/python/test_pymsnumpress.py` around lines 3 - 7, The module docstring in test_pymsnumpress.py identifies the wrong file; update its filename entry from setup.py to test_pymsnumpress.py while preserving the remaining header text.
99-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
assertAlmostEqualon two lists fails with aTypeError, not a clear assertion.Lines 101 and 113 pass lists to
assertAlmostEqual.unittestfirst triesfirst == secondand returns early on equality. If the lists differ, it computesround(first - second, 7), and list subtraction raisesTypeError. The test then reports a type error instead of the numeric difference. UseassertEqualfor the exact case, or compare element by element.♻️ Proposed change
self.assertEqual(len(result), 4) self.assertAlmostEqual(result[0], 100) - self.assertAlmostEqual(result, self.data) + for actual, expected in zip(result, self.data): + self.assertAlmostEqual(actual, expected)Apply the same change at line 113.
🤖 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 `@subprojects/msnumpress/src/main/python/test_pymsnumpress.py` around lines 99 - 113, Update the list comparisons in the affected tests, including test_encodePic, to use assertEqual for exact list equality or compare corresponding numeric elements individually with assertAlmostEqual; keep the existing scalar result[0] tolerance checks unchanged.subprojects/msnumpress/src/main/csharp/MSNumpressTest.cs (2)
1-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the upstream provenance for the vendored MS-Numpress sources.
This file is an unmodified copy of the upstream MS-Numpress C# test suite. The subproject vendors Java, C#, Python, and R sources that the mzpeak build does not compile. Record the upstream repository URL and the exact commit or release tag in
subprojects/msnumpress/README.md. A recorded pin makes later upstream syncs and license audits straightforward.The static analysis warnings about
System.Randomin this file are not applicable. The random values are synthetic m/z and intensity test data. They are not security-sensitive.🤖 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 `@subprojects/msnumpress/src/main/csharp/MSNumpressTest.cs` around lines 1 - 24, Update subprojects/msnumpress/README.md to document the upstream MS-Numpress repository URL and the exact commit or release tag corresponding to the vendored Java, C#, Python, and R sources, preserving the existing test file unchanged.Source: Linters/SAST tools
174-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead computation in
encodeDecodeLinear.Line 191 builds
listand never asserts on it. The expression also hardcodes1000instead ofn. Remove the line, or replace it with an assertion on the ratio.♻️ Proposed change
Assert.AreEqual(n, decodedDoubles); - var list = Enumerable.Range(0, 1000).Select(i => decoded[i] / mzs[i]).ToList(); - for (int i = 0; i < n; i++) Assert.AreEqual(mzs[i], decoded[i], 0.000005);Removing line 191 also makes the
using System.Linq;import at line 28 unused.🤖 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 `@subprojects/msnumpress/src/main/csharp/MSNumpressTest.cs` around lines 174 - 195, Remove the unused list computation from encodeDecodeLinear, including the hardcoded Enumerable.Range expression; retain the existing decoded-value assertions and remove the now-unused System.Linq import.include/mzpeak/data/transformer/primary.h (1)
124-134: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle unhandled chunk encodings explicitly
C++23 is enabled. Add a direct
<utility>include forstd::unreachable, but replace the fallback withInvalidFormatError. IfChunkEncoding::Typegains a value, the current fallback causes undefined behavior instead of a diagnosable error.🤖 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 `@include/mzpeak/data/transformer/primary.h` around lines 124 - 134, In the switch handling ChunkEncoding::Type, add a direct <utility> include for std::unreachable, then replace the trailing std::unreachable() fallback with InvalidFormatError so unsupported encoding values produce a diagnosable format error while preserving the existing cases.subprojects/msnumpress/src/main/csharp/MSNumpress.cs (1)
28-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the class inside a namespace.
MSNumpressand the nestedIntDecoderare declared in the global namespace. Any consumer that references this file gets both types without an import, and the common nameMSNumpresscan collide with another vendored copy. Wrap the class in a namespace such asMS.Numpress, which matches the Java packagems.numpressand the C++ namespacems::numpress.🤖 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 `@subprojects/msnumpress/src/main/csharp/MSNumpress.cs` around lines 28 - 29, Wrap the MSNumpress class and its nested IntDecoder type in a dedicated MS.Numpress namespace, matching the Java and C++ package conventions, and add the necessary closing scope without changing their existing behavior.subprojects/msnumpress/meson.build (1)
14-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused static dependency and review vendored-library installation.
The parent project consumes only
msnumpres_cpp_lib_so. The static library is used only byMSNumpressTest, andmsnumpres_cpp_lib_ahas no consumer. Remove the unused exported static dependency. Useboth_libraries()only if another consumer requires both variants.Keep
install : trueforcpp_lib_soif the installedmzpeakshared library requireslibnumpressat runtime. Confirm that installingMSNumpress.hppas a public header is intentional.🤖 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 `@subprojects/msnumpress/meson.build` around lines 14 - 38, Remove the unused msnumpres_cpp_lib_a declare_dependency and retain cpp_lib_a only for linking MSNumpressTest. Keep msnumpres_cpp_lib_so and cpp_lib_so installation if the installed mzpeak shared library requires libnumpress at runtime; do not switch to both_libraries unless another consumer needs both variants. Verify that the existing MSNumpress.hpp public-header installation is intentional.subprojects/msnumpress/src/main/R/RMSNumpress/man/decodeLinear.Rd (1)
12-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
\valueblocks of the three decode help pages document the C pointer API, not the R return value. Each R decode function returns a numeric vector of decoded values. None returns a count, and none returns-1; the C++ vector overloads throw on corrupt input. These.Rdfiles are roxygen-generated, so correct the@returntags insubprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cppand regenerate.
subprojects/msnumpress/src/main/R/RMSNumpress/man/decodeLinear.Rd#L12-L14: state that the function returns a numeric vector of decoded doubles and that corrupt input raises an error.subprojects/msnumpress/src/main/R/RMSNumpress/man/decodePic.Rd#L12-L14: state that the function returns a numeric vector of decoded doubles.subprojects/msnumpress/src/main/R/RMSNumpress/man/decodeSlof.Rd#L12-L14: state that the function returns a numeric vector of decoded doubles.🤖 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 `@subprojects/msnumpress/src/main/R/RMSNumpress/man/decodeLinear.Rd` around lines 12 - 14, Update the `@return` tags for the R decode functions in RMSNumpress.cpp, then regenerate the documentation: decodeLinear should document a numeric vector of decoded doubles and an error for corrupt input, while decodePic and decodeSlof should document numeric vectors of decoded doubles. Regenerated documentation must update decodeLinear.Rd, decodePic.Rd, and decodeSlof.Rd accordingly; no direct manual changes to the generated files are required.subprojects/msnumpress/src/main/R/RMSNumpress/man/RMSNumpress-package.Rd (1)
14-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo documentation inaccuracies in the package description.
Line 14 says "two compression schemes", and Line 16 says "3 different algorithms". Line 53 states
Xres = Xpred - X(n+1), butencodeLinearinsubprojects/msnumpress/src/main/R/RMSNumpress/src/MSNumpress.cppLine 327 computesdiff = ints[2] - extrapol, which isX(n+1) - Xpred. Correct the count and the residual sign.📝 Proposed change
-Implementations of two compression schemes for numeric data from mass spectrometers. +Implementations of compression schemes for numeric data from mass spectrometers. @@ -Xres = Xpred - X(n+1) +Xres = X(n+1) - Xpred🤖 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 `@subprojects/msnumpress/src/main/R/RMSNumpress/man/RMSNumpress-package.Rd` around lines 14 - 53, Correct the RMSNumpress package description to state that the library provides three compression schemes/algorithms, and update the Numpress Lin residual equation to use the sign implemented by encodeLinear: Xres = X(n+1) - Xpred. Leave the surrounding algorithm descriptions unchanged.subprojects/msnumpress/pom.xml (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRaise the Java release level for Maven builds.
Java 6 source and target levels fail on JDK 12 and later. The Meson build remains C++-only, but the README documents Maven Java tests. If Java 8 compatibility is acceptable, update the compiler plugin and use
<release>8</release>.🤖 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 `@subprojects/msnumpress/pom.xml` around lines 23 - 30, Update the Maven compiler configuration in the compiler plugin to use a Java 8 release level instead of the obsolete 1.6 source and target settings, and select a compiler-plugin version that supports the release option so Maven builds work on JDK 12 and later.include/mzpeak/data/transformer/secondary.h (1)
16-18: 📐 Maintainability & Code Quality | 🔵 TrivialDocument
Secondary::Decoder.Replace the FIXME with the expected chunk input shape, transform behavior, and
result_typealternatives. I can draft this API documentation.🤖 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 `@include/mzpeak/data/transformer/secondary.h` around lines 16 - 18, Replace the FIXME comment above Secondary::Decoder with API documentation covering the expected chunk input shape, the decoder’s transform behavior, and the possible result_type alternatives.include/mzpeak/util/numpress.h (1)
39-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
<type_traits>and take the argument by const reference.
castusesstd::is_same_vbut the file does not include<type_traits>. The header compiles today only because<arrow/array.h>includes it transitively. Include it directly.
castnever modifiesdoubles. The non-const lvalue reference also prevents callers from passing a temporary.♻️ Proposed change
`#include` <arrow/array.h> `#include` <cstdint> `#include` <memory> +#include <type_traits> `#include` <vector>template <typename T> -std::shared_ptr<std::vector<T>> cast(std::shared_ptr<std::vector<double>>& doubles) +std::shared_ptr<std::vector<T>> +cast(const std::shared_ptr<std::vector<double>>& doubles)🤖 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 `@include/mzpeak/util/numpress.h` around lines 39 - 41, Update the header to include <type_traits> directly, and change the cast template’s doubles parameter to a const reference since it is not modified. Preserve the existing cast behavior while allowing const objects and temporaries as arguments.src/util/decoders.cpp (2)
18-26: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the
possibly_numpress()lookup out of the per-element lambda.
countruns once per valid outer list element throughfor_list. Each call re-evaluatescolumn.second->possibly_numpress(), which performs a substring search on the schema name.The result depends only on
column, which does not change during the call.Slice::arrayline 150 invokesguess_array_lengthfor every chunk of every column read, so this repeats on the decode hot path.⚡ Proposed change
std::size_t guess_array_length(const Schema::Column& column, const std::shared_ptr<arrow::Array>& ary) { - auto count = [&column](const std::shared_ptr<arrow::Array>& nums) -> std::size_t { - std::optional<Numpress::Type> numpress = column.second->possibly_numpress(); + const std::optional<Numpress::Type> numpress = column.second->possibly_numpress(); + auto count = [&numpress](const std::shared_ptr<arrow::Array>& nums) -> std::size_t { if (numpress.has_value()) { return Numpress::decoding_space_needed(nums->length(), numpress.value()); } else { return static_cast<std::size_t>(nums->length()); } };🤖 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 `@src/util/decoders.cpp` around lines 18 - 26, Move the column.second->possibly_numpress() lookup out of the count lambda and compute it once before the lambda is created. Capture and reuse that stable optional Numpress type inside count, preserving the existing decoding_space_needed versus nums->length behavior.
28-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the Arrow list dispatch with
Decoders::visit.This function and
Decoders::visitininclude/mzpeak/util/decoders.hlines 66-82 both dispatch over the same five Arrow list types with the samestatic_pointer_castchain. Both were added in this change.The two differ only in the non-list branch and in what they accumulate.
visitthrowsTypeError; this returnscount(ary). When a new Arrow list type is added, or a cast is corrected, the two tables can diverge without a compile error.A shared helper that performs the cast and invokes a generic callable would leave each caller with only its own accumulation and fallback logic.
🤖 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 `@src/util/decoders.cpp` around lines 28 - 55, Share the five-type Arrow list dispatch between this counting function and Decoders::visit in the decoders header. Extract or reuse a generic helper that performs each static_pointer_cast and invokes the supplied callable, while preserving this function’s count(ary) fallback and visit’s TypeError behavior; keep accumulation logic in each caller.src/schema/group.cpp (1)
125-134: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueInclude SLOF and PIC in the array-length hint.
possibly_numpress()only reportsLinear.guess_array_length()therefore reserves the raw byte count beforeSecondary::Decoderapplies SLOF or PIC. PIC usesn * 2decoded space and can cause avoidable vector reallocations.Pass
ArrayIndex::Dimension::transforminto the length-hint path instead of adding more schema-name matching.🤖 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 `@src/schema/group.cpp` around lines 125 - 134, Update Group::Field::possibly_numpress() and its guess_array_length() caller to accept and propagate ArrayIndex::Dimension::transform, so the array-length hint accounts for SLOF and PIC transformations rather than relying on additional schema-name matching. Preserve the existing Linear behavior while ensuring PIC’s expanded decoded size is reflected before Secondary::Decoder runs.
🤖 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 `@bin/mzp-inspect.cpp`:
- Around line 159-161: Before the inner loop in the spectrum inspection flow,
validate that the decoded intensity array is at least as long as the mz array
used by the std::views::iota range. Reject mismatched arrays before the loop so
intensity[row] in the existing output statement is always safely indexed.
In `@include/mzpeak/util/algorithm.h`:
- Around line 143-151: Update the null-handling logic around the
`casted->IsNull(0)` check so the leading-null rule applies whenever the array is
non-empty, including `length == 1`; separately guard access to
`casted->IsNull(1)` with the length check. Ensure a single null produces only
`[null]`, and add `algorithm_test.cpp` coverage for a single null element, one
leading null, and two leading nulls.
In `@include/mzpeak/util/numpress.h`:
- Around line 27-31: Update decoding_space_needed in numpress.cpp to clamp n
before applying the (n - 8) calculations for Linear and SLOF, preventing
unsigned underflow for inputs shorter than 8 bytes and preserving safe
reservation sizes for truncated data. Document this minimum-length behavior or
precondition in the decoding_space_needed declaration comment in numpress.h.
In `@include/mzpeak/util/types.h`:
- Around line 106-170: Add the Arrow builder definitions header to support the
builder_type aliases used by each type_traits specialization, and update the
trait documentation to explicitly describe builder_type. Limit the change to the
includes and comment in types.h.
In `@src/util/numpress.cpp`:
- Around line 47-51: Before invoking Decoders::Scalar<uint8_t>::decode in the
compressed-input path, check src->null_count(); when it is nonzero, throw
InvalidFormatError and do not decode. Preserve the existing reserve and decode
behavior for inputs without null bytes.
- Around line 23-32: Update decoding_space_needed to reject input lengths below
the eight-byte fixed-point header before any subtraction, preventing unsigned
underflow for Linear and SLOF. Also add overflow checks before the Linear and
PIC multiplications, returning the existing invalid/error result used by callers
when capacity cannot be safely computed.
In `@subprojects/msnumpress/src/main/cpp/MSNumpress.cpp`:
- Around line 742-752: Update decodeSlof to reject odd dataSize values before
the loop that reads data[i+1], returning the same failure status used for
corrupt input (as in the C# port). Alternatively, add this validation in the
mzpeak wrapper, ensuring malformed odd-length input never reaches the
out-of-bounds read; if patching the vendored source, document the patch.
- Around line 451-459: Add validation in the mzpeak wrapper layer before calling
the vector overloads or decoding_space_needed(): reject Linear and SLOF buffers
shorter than 8 bytes, reject SLOF payloads with odd length, and map these
malformed inputs to InvalidFormatError. Handle empty PIC input by returning an
empty result before taking element addresses; do not modify the vendored
MSNumpress.cpp implementations.
In `@subprojects/msnumpress/src/main/cpp/MSNumpress.hpp`:
- Around line 127-157: Update the vector overload of decodeLinear to validate
data.size() before calculating or applying the output resize based on (dataSize
- 8) * 2. For inputs smaller than 8 bytes, avoid resizing and delegate to the
lower-level decodeLinear so its existing invalid-input behavior is preserved.
In `@subprojects/msnumpress/src/main/cpp/MSNumpressTest.cpp`:
- Around line 762-786: Update main in MSNumpressTest.cpp to invoke
decodeLinearWierd_llong_overflow, decodeLinearWierd_int_overflow, and
decodeLinearWierd_int_underflow alongside the existing decoding tests. Update
the test executable configuration in meson.build to force assertions enabled
with cpp_args ['-UNDEBUG'] (or equivalent b_ndebug=false), ensuring these tests
continue validating behavior in release builds.
In `@subprojects/msnumpress/src/main/csharp/MSNumpress.cs`:
- Around line 90-96: Update the SLOF branch of decode to reject dataSize values
below 8 with the same documented ArgumentException behavior used by the LINEAR
and PIC branches, then check the return value from MSNumpress.decodeSlof and
throw ArgumentException when decoding fails, including odd-sized input. Return
the result only after successful validation and decoding.
In `@subprojects/msnumpress/src/main/java/ms/numpress/MSNumpress.java`:
- Around line 62-66: Update the ACC_NUMPRESS_SLOF branch in MSNumpress.decode to
reject dataSize values below 8 with IllegalArgumentException, matching the
LINEAR and PIC branches. Also validate the decodeSlof return value and propagate
an appropriate failure for odd or otherwise invalid input instead of returning a
zero-filled result.
In `@subprojects/msnumpress/src/main/python/PyMSNumpress.pyx`:
- Around line 72-101: Add an early empty-input guard to
optimalLinearFixedPointMass, optimalLinearFixedPoint, and optimalSlofFixedPoint
before taking &c_data[0], returning 0 for empty data; preserve the existing
vector conversion and native-function calls for non-empty inputs.
In `@subprojects/msnumpress/src/main/R/RMSNumpress/src/MSNumpress.cpp`:
- Around line 762-772: Update decodeSlof to reject odd dataSize values before
the loop reads fixed-size pairs, returning -1 as the Java implementation does.
Preserve the existing insufficient-length exception for dataSize values below 8,
and ensure the even-length decoding path remains unchanged.
- Around line 451-459: Guard the vector overloads decodeLinear and decodeSlof
against inputs shorter than the fixed 8-byte header before subtracting from
data.size(), and throw the established descriptive corrupt-input error. Ensure
empty input never reaches &data[0], while preserving the existing resize/decode
flow for valid inputs.
- Around line 20-33: Apply the decodeSlof bounds fix consistently in both
MSNumpress.cpp codec implementations: update each vector overload to avoid
unchecked dataSize - 8 arithmetic and guard insufficient input before
subtracting or indexing. Preserve the existing decoding behavior for valid
buffers in both the Meson C++ and R package implementations.
In `@subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp`:
- Around line 36-42: Replace unsafe &data[0] access with data.data() or nullptr
for empty vectors in optimalLinearFixedPointMass
(subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp:36-42),
optimalLinearFixedPoint (same file:53-56), and optimalSlofFixedPoint (same
file:67-70), preserving the native helpers’ empty-input behavior of returning 0.
---
Minor comments:
In `@include/mzpeak/data/array_index.h`:
- Around line 212-213: Initialize the `layout_` member in the default
constructor of the relevant array index class to `Layout::Unknown`, ensuring
subsequent `layout()` calls report an unknown layout until explicitly set.
In `@include/mzpeak/schema/group.h`:
- Around line 123-128: Update the documentation for Group’s possibly_numpress()
method to state that it returns the detected Util::Numpress::Type in the
optional, or no value when the column name does not indicate Numpress
compression, instead of claiming it returns true.
In `@include/mzpeak/util/decoders.h`:
- Around line 220-233: Update the inner decoder type in List::decode so
scalar_decoder_ is instantiated with value_type rather than the outer container
type C, allowing constrained C alternatives to compile while retaining the
existing res decoding flow. Also update the CRTP base types for List and
Flattened to include their N template parameter, ensuring different List<V, C,
N> and Flattened<V, C, N> instantiations do not share a base type.
In `@include/mzpeak/util/numpress.h`:
- Around line 72-78: Remove the trailing semicolon after the
decode_linear_convert function body, leaving the definition terminated with `}`
like the sibling decode_slof_convert and decode_pic_convert functions.
In `@subprojects/msnumpress/src/main/cpp/MSNumpressTest.cpp`:
- Around line 161-187: Update the loop bound in the fixed-point accuracy test to
iterate all five entries in mz_err and encodedLength, including the 1e-7 case
and its expected encoded length.
In `@subprojects/msnumpress/src/main/csharp/MSNumpress.cs`:
- Around line 365-369: Update the trailing half-byte validation in the affected
decode loop to match decodeLinear and decodePic: when dec.pos is the final byte
and dec.half is set, break only if the low nibble is 0x0. Preserve 0x8 as a
valid zero-value head and reject malformed nonzero trailing nibbles such as 0x3
instead of silently truncating.
In `@subprojects/msnumpress/src/main/java/ms/numpress/MSNumpress.java`:
- Around line 198-209: Confirm the upstream ms-numpress implementation’s
single-element fixed-point behavior across the vendored Java and C++ codecs
before editing. Then update the single-element branch in the Java fixed-point
calculation to use the same signed 32-bit range as the C++ implementation and
preserve agreement with encodeLinear’s signed 4-byte storage.
In `@subprojects/msnumpress/src/main/python/setup.py`:
- Around line 36-48: Update setup.py’s native source handling to resolve paths
relative to os.path.dirname(__file__) for both the MSNumpress.cpp/MSNumpress.hpp
copy operations and the Extension sources, so builds work from any working
directory. Remove the broad IOError suppression and allow copy failures to
propagate.
In `@subprojects/msnumpress/src/main/R/RMSNumpress/inst/LICENSE.md`:
- Around line 1-19: Update the BSD 3-Clause license notice in RMSNumpress to
replace the literal <ORGANIZATION> placeholder with the appropriate organization
name, and explicitly document that the vendored codec sources in MSNumpress.cpp
remain licensed under Apache 2.0 by Johan Teleman. Preserve the existing BSD
terms while clearly recording the dual licensing for the bundled sources.
In `@subprojects/msnumpress/src/main/R/RMSNumpress/man/encodeLinear.Rd`:
- Around line 16-18: Update the roxygen `@return` tags for the three encode
functions in RMSNumpress.cpp to describe the returned raw vector of encoded
bytes rather than a byte count, then regenerate the documentation. Apply the
generated change to encodeLinear.Rd (lines 16-18), encodePic.Rd (lines 12-14),
and encodeSlof.Rd (lines 14-16), with no other documentation changes.
In
`@subprojects/msnumpress/src/main/R/RMSNumpress/man/optimalLinearFixedPointMass.Rd`:
- Around line 20-25: Update the note for optimalLinearFixedPointMass in the
roxygen source to state that overflow occurs with 32-bit integers, then
regenerate the corresponding RMSNumpress documentation file so the generated
text matches.
In `@subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp`:
- Around line 103-108: Update the Roxygen return documentation for encodeLinear
(subprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cpp:103-108),
decodeLinear (141-145), encodeSlof (162-167), decodeSlof (194-198), encodePic
(214-218), and decodePic (235-239) to describe encoded functions as returning
raw vectors and decoded functions as returning numeric vectors, replacing scalar
count descriptions. Regenerate RcppExports.R and the Rd files afterward.
In `@subprojects/README.md`:
- Around line 16-19: Replace the fenced command blocks with the configured
indented Markdown code-block style in subprojects/README.md lines 16-19,
subprojects/msnumpress/README.md lines 16-18, 23-25, 28-30, and 67-69; preserve
each command and its formatting as executable text.
In `@test/spectra_test.cpp`:
- Around line 29-44: In the test around spectra and m/z validation, replace the
non-fatal BOOST_TEST checks for the spectrum count and mz size with
BOOST_TEST_REQUIRE. Keep the existing expected values and tolerance checks
unchanged so indexing spectra[0] and the first/last m/z elements only occurs
after the collections are confirmed non-empty.
---
Nitpick comments:
In `@include/mzpeak/data/transformer/primary.h`:
- Around line 124-134: In the switch handling ChunkEncoding::Type, add a direct
<utility> include for std::unreachable, then replace the trailing
std::unreachable() fallback with InvalidFormatError so unsupported encoding
values produce a diagnosable format error while preserving the existing cases.
In `@include/mzpeak/data/transformer/secondary.h`:
- Around line 16-18: Replace the FIXME comment above Secondary::Decoder with API
documentation covering the expected chunk input shape, the decoder’s transform
behavior, and the possible result_type alternatives.
In `@include/mzpeak/util/numpress.h`:
- Around line 39-41: Update the header to include <type_traits> directly, and
change the cast template’s doubles parameter to a const reference since it is
not modified. Preserve the existing cast behavior while allowing const objects
and temporaries as arguments.
In `@src/schema/group.cpp`:
- Around line 125-134: Update Group::Field::possibly_numpress() and its
guess_array_length() caller to accept and propagate
ArrayIndex::Dimension::transform, so the array-length hint accounts for SLOF and
PIC transformations rather than relying on additional schema-name matching.
Preserve the existing Linear behavior while ensuring PIC’s expanded decoded size
is reflected before Secondary::Decoder runs.
In `@src/util/decoders.cpp`:
- Around line 18-26: Move the column.second->possibly_numpress() lookup out of
the count lambda and compute it once before the lambda is created. Capture and
reuse that stable optional Numpress type inside count, preserving the existing
decoding_space_needed versus nums->length behavior.
- Around line 28-55: Share the five-type Arrow list dispatch between this
counting function and Decoders::visit in the decoders header. Extract or reuse a
generic helper that performs each static_pointer_cast and invokes the supplied
callable, while preserving this function’s count(ary) fallback and visit’s
TypeError behavior; keep accumulation logic in each caller.
In `@subprojects/msnumpress/meson.build`:
- Around line 14-38: Remove the unused msnumpres_cpp_lib_a declare_dependency
and retain cpp_lib_a only for linking MSNumpressTest. Keep msnumpres_cpp_lib_so
and cpp_lib_so installation if the installed mzpeak shared library requires
libnumpress at runtime; do not switch to both_libraries unless another consumer
needs both variants. Verify that the existing MSNumpress.hpp public-header
installation is intentional.
In `@subprojects/msnumpress/pom.xml`:
- Around line 23-30: Update the Maven compiler configuration in the compiler
plugin to use a Java 8 release level instead of the obsolete 1.6 source and
target settings, and select a compiler-plugin version that supports the release
option so Maven builds work on JDK 12 and later.
In `@subprojects/msnumpress/src/main/csharp/MSNumpress.cs`:
- Around line 28-29: Wrap the MSNumpress class and its nested IntDecoder type in
a dedicated MS.Numpress namespace, matching the Java and C++ package
conventions, and add the necessary closing scope without changing their existing
behavior.
In `@subprojects/msnumpress/src/main/csharp/MSNumpressTest.cs`:
- Around line 1-24: Update subprojects/msnumpress/README.md to document the
upstream MS-Numpress repository URL and the exact commit or release tag
corresponding to the vendored Java, C#, Python, and R sources, preserving the
existing test file unchanged.
- Around line 174-195: Remove the unused list computation from
encodeDecodeLinear, including the hardcoded Enumerable.Range expression; retain
the existing decoded-value assertions and remove the now-unused System.Linq
import.
In `@subprojects/msnumpress/src/main/python/test_pymsnumpress.py`:
- Around line 3-7: The module docstring in test_pymsnumpress.py identifies the
wrong file; update its filename entry from setup.py to test_pymsnumpress.py
while preserving the remaining header text.
- Around line 99-113: Update the list comparisons in the affected tests,
including test_encodePic, to use assertEqual for exact list equality or compare
corresponding numeric elements individually with assertAlmostEqual; keep the
existing scalar result[0] tolerance checks unchanged.
In `@subprojects/msnumpress/src/main/R/RMSNumpress/man/decodeLinear.Rd`:
- Around line 12-14: Update the `@return` tags for the R decode functions in
RMSNumpress.cpp, then regenerate the documentation: decodeLinear should document
a numeric vector of decoded doubles and an error for corrupt input, while
decodePic and decodeSlof should document numeric vectors of decoded doubles.
Regenerated documentation must update decodeLinear.Rd, decodePic.Rd, and
decodeSlof.Rd accordingly; no direct manual changes to the generated files are
required.
In `@subprojects/msnumpress/src/main/R/RMSNumpress/man/RMSNumpress-package.Rd`:
- Around line 14-53: Correct the RMSNumpress package description to state that
the library provides three compression schemes/algorithms, and update the
Numpress Lin residual equation to use the sign implemented by encodeLinear: Xres
= X(n+1) - Xpred. Leave the surrounding algorithm descriptions unchanged.
In `@test/algorithm_test.cpp`:
- Around line 18-51: Add a separate test alongside
null_delta_decode_with_no_nulls that builds an Arrow Int64 input containing one
or more null entries, invokes Algorithm::null_delta_decode<Type::Int64>, and
verifies both output null preservation and decoded values after nulls according
to the null-delta contract. Reuse the existing Arrow builder and assertion
style, including coverage of accumulator behavior following a null entry.
🪄 Autofix
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 Plus
Run ID: 463ce517-ee63-451a-a014-004f0872eeae
📒 Files selected for processing (64)
.gitignorebin/mzp-inspect.cppinclude/mzpeak/data/array_index.hinclude/mzpeak/data/encoding.hinclude/mzpeak/data/signals.hinclude/mzpeak/data/transformer/primary.hinclude/mzpeak/data/transformer/secondary.hinclude/mzpeak/exception.hinclude/mzpeak/schema/group.hinclude/mzpeak/schema/psi/chunk_encoding.hinclude/mzpeak/schema/psi/transform.hinclude/mzpeak/util/algorithm.hinclude/mzpeak/util/decoders.hinclude/mzpeak/util/numpress.hinclude/mzpeak/util/slice.hinclude/mzpeak/util/types.hmeson.buildsrc/data/array_index.cppsrc/data/signals.cppsrc/schema/group.cppsrc/schema/psi/chunk_encoding.cppsrc/schema/psi/transform.cppsrc/util/decoders.cppsrc/util/numpress.cppsubprojects/README.mdsubprojects/msnumpress/README.mdsubprojects/msnumpress/meson.buildsubprojects/msnumpress/pom.xmlsubprojects/msnumpress/src/main/R/RMSNumpress/DESCRIPTIONsubprojects/msnumpress/src/main/R/RMSNumpress/LICENSEsubprojects/msnumpress/src/main/R/RMSNumpress/NAMESPACEsubprojects/msnumpress/src/main/R/RMSNumpress/R/RcppExports.Rsubprojects/msnumpress/src/main/R/RMSNumpress/inst/LICENSE.mdsubprojects/msnumpress/src/main/R/RMSNumpress/man/RMSNumpress-package.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/decodeLinear.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/decodePic.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/decodeSlof.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/encodeLinear.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/encodePic.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/encodeSlof.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/optimalLinearFixedPoint.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/optimalLinearFixedPointMass.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/man/optimalSlofFixedPoint.Rdsubprojects/msnumpress/src/main/R/RMSNumpress/src/MSNumpress.cppsubprojects/msnumpress/src/main/R/RMSNumpress/src/RMSNumpress.cppsubprojects/msnumpress/src/main/R/RMSNumpress/src/RcppExports.cppsubprojects/msnumpress/src/main/R/RMSNumpress/src/include/MSNumpress.hppsubprojects/msnumpress/src/main/R/RMSNumpress/tests/testthat.Rsubprojects/msnumpress/src/main/R/RMSNumpress/tests/testthat/test_RMSNumpress.Rsubprojects/msnumpress/src/main/cpp/MSNumpress.cppsubprojects/msnumpress/src/main/cpp/MSNumpress.hppsubprojects/msnumpress/src/main/cpp/MSNumpressTest.cppsubprojects/msnumpress/src/main/csharp/MSNumpress.cssubprojects/msnumpress/src/main/csharp/MSNumpressTest.cssubprojects/msnumpress/src/main/java/ms/numpress/IntDecoder.javasubprojects/msnumpress/src/main/java/ms/numpress/MSNumpress.javasubprojects/msnumpress/src/main/python/MSNumpress.pxdsubprojects/msnumpress/src/main/python/PyMSNumpress.pyxsubprojects/msnumpress/src/main/python/setup.pysubprojects/msnumpress/src/main/python/test_pymsnumpress.pysubprojects/msnumpress/src/test/java/ms/numpress/MSNumpressTest.javatest/algorithm_test.cpptest/array_index_test.cpptest/spectra_test.cpp
- Improve length guessing when numpress is used - Deal with all of the list types
Taken from upstream: - The MS-Numpress wrappers. Theirs are a strict superset of ours -- a Type enum, arrow::Array overloads, and decode_pic, the PIC codec (MS:1002313) our own coverage review had flagged as unwrapped and unreachable. Ours (linear + slof only) is dropped, along with the vendored src/util/vendor/MSNumpress.cpp, in favour of their subprojects/msnumpress subproject. Our numpress tests are KEPT and adapted to the new API: they pin real bytes against reference values, so they now serve as the oracle for upstream's implementation, and they pass. - The new transformer/, chunk_encoding, exception, array_index and decoders work, plus their mzp-inspect spectra dump. - signals.cpp: their field_name form. Ours replaced spaces with underscores defensively; entity_type_to_string already returns the underscore spelling the specification canonicalised in mzPeak-specification#18, and field() no longer exists on Signals, so our side would not have compiled. - algorithm.h was a pure add/add of two DIFFERENT functions -- our intersect_range and their null_delta_decode. Both kept. Not taken -- the dimension-dispatch rewrite in encoding.h: Upstream now dispatches on ArrayIndex::Layout (Point/Chunked/Unknown). Ours dispatches on the entries and additionally handles the coalesced-point case -- several complementary point columns describing ONE logical array, which is what has_uv does with counts vs absorbance intensity -- and notes the unit so intensity_unit() can answer. Both are covered by tests theirs would not satisfy, so ours is kept. Upstream's null-marking expectations are WRONG, and this is worth reporting: their new can_read_spectra asserts mz[8] == 202.60856 and mz[15] == 204.762. Nulls come in pairs separating two runs; the first continues the run on its left, the second belongs to the run on its right. The raw column is row 6 202.60806612940473, rows 7-8 null, row 9 204.75958873936264 -- the runs are ~2.15 apart -- so position 8 is 204.75933, not 202.60856. Their values are what extrapolating BOTH nulls forward from the left run produces, which is the defect this tree fixed earlier, and they disagree with the reference reader (hupo-mzpeak examples/read_spectrum small.mzpeak 0 emits 204.75933490116418). Their two constants are corrected here, with the raw rows recorded in the test. Their mz[7] and mz[14] -- the FIRST null of each pair -- were already right. 36/36 in release, debug (-Dwerror) and TSan; run2k digest 9fae6e76aea6a57e unchanged, scalar and batch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation