Implement full "point" decoding by processing "null marking" transformations#11
Conversation
Also incorporate the dimension struct into the array index where it belongs.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughChangesThe PR replaces untyped dimension transforms with PSI transform objects, introduces grouped Typed spectrum decoding
Sequence Diagram(s)sequenceDiagram
participant Spectra
participant Signals
participant Spectrum
participant Decoder
Spectra->>Signals: select projected dimensions
Signals-->>Spectra: return slice
Spectra->>Spectrum: construct spectrum
Spectrum->>Decoder: decode m/z and intensity
Decoder-->>Spectrum: return decoded arrays
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/data/array_index.cpp (1)
88-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnsure deterministic ordering of array entries.
std::ranges::sortis not stable, so sorting solely byarray_namecan scramble the relative order of entries that share the samearray_name(e.g., sequentially chunked arrays mapped to the same dimension). Consider addingnameas a secondary sort key to guarantee a deterministic chunk order prior to grouping.🐛 Proposed fix
- std::ranges::sort(entries_, {}, &Entry::array_name); + std::ranges::sort(entries_, [](const auto& a, const auto& b) { + if (a.array_name != b.array_name) return a.array_name < b.array_name; + return a.name < b.name; + });🤖 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/data/array_index.cpp` at line 88, Update the entries_ sort in the array-index construction path to use array_name as the primary key and name as a secondary key, ensuring entries sharing an array_name have deterministic chunk ordering before grouping.
🧹 Nitpick comments (5)
test/array_index_test.cpp (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
.value()over*to prevent potential undefined behavior.
BOOST_TESTdoes not halt execution on failure. IfSchema::CV::from_stringwere to fail in the future,*transform_cvwould invoke undefined behavior. Using.value()will throwstd::bad_optional_accessinstead, safely failing the test.♻️ Proposed refactor
std::optional<Schema::CV> transform_cv(Schema::CV::from_string("MS:1003901")); BOOST_TEST((transform_cv.has_value())); - std::optional<Schema::PSI::Transform> transform(*transform_cv); + std::optional<Schema::PSI::Transform> transform(transform_cv.value());🤖 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/array_index_test.cpp` around lines 34 - 36, Update the construction of Schema::PSI::Transform from transform_cv to use transform_cv.value() instead of dereferencing it with *. Keep the existing BOOST_TEST assertion and surrounding test flow unchanged.include/mzpeak/util/algorithm.h (1)
61-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant sort before
boost::math::statistics::median.Boost's
median()is a wrapper aroundstd::nth_element, which doesn't require (or benefit from) pre-sorted input. The manualstd::ranges::sort(ds)here is redundant O(n log n) work on the decode hot path (invoked per null-run boundary during spectrum decoding).♻️ Suggested simplification
std::vector<T> ds(deltas(values)); if (ds.empty()) return {}; - std::ranges::sort(ds); T median = boost::math::statistics::median(ds.begin(), ds.end());🤖 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/algorithm.h` around lines 61 - 76, Remove the redundant std::ranges::sort call from median_delta and rely on boost::math::statistics::median for the initial median calculation. Preserve the existing delta filtering, empty-result handling, and final median behavior.src/schema/psi/transform.cpp (1)
67-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix unconditionally evaluated
static_assert.The
static_assert(false)in theelsebranch of theif constexprwill evaluate unconditionally and fail compilation on compilers that haven't fully implemented CWG2518 from C++23 (such as Clang 14, as indicated by the static analysis error).To ensure compatibility across a broader range of compilers, make the assertion dependent on the template type
T.♻️ Proposed refactor
return std::visit( [](auto&& arg) -> CV { using T = std::decay_t<decltype(arg)>; if constexpr (std::is_same_v<T, Type>) { return type_to_cv(arg); } else if constexpr (std::is_same_v<T, CV>) { return arg; } else { - static_assert(false, "variant not handled"); + static_assert(sizeof(T) == 0, "variant not handled"); } }, val_);🤖 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/psi/transform.cpp` around lines 67 - 78, Update the fallback branch in the std::visit lambda within the CV conversion flow to make the static assertion dependent on the template type T, preventing unconditional evaluation on older compilers while retaining the compile-time failure for unhandled variant alternatives.src/data/array_index.cpp (1)
19-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typo in variable name.
💡 Proposed change
bool ArrayIndex::Dimension::needs_delta_model() const { - bool from_tansform = transform.has_value() && transform->needs_delta_model(); - return from_tansform || std::ranges::any_of(entries, [](const auto& e) { + bool from_transform = transform.has_value() && transform->needs_delta_model(); + return from_transform || std::ranges::any_of(entries, [](const auto& e) { return e.sorting_rank.has_value() && e.sorting_rank.value() == 0; }); }🤖 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/data/array_index.cpp` around lines 19 - 25, Rename the misspelled local variable from_tansform to from_transform within ArrayIndex::Dimension::needs_delta_model, updating its declaration and return expression while preserving the existing logic.src/spectrum.cpp (1)
25-27: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid copying the
shared_ptr.
datais passed to the constructor by value, meaning it already owns its reference count. You can safelystd::moveit intodecoder_to save an unnecessary atomic reference count increment and decrement.♻️ Proposed refactor
- , decoder_(data, + , decoder_(std::move(data), std::move(slice), Util::DeltaEstimator<double>(md_spec_.delta_model()))🤖 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/spectrum.cpp` around lines 25 - 27, Update the decoder_ initialization in the Spectrum constructor to move the by-value data shared_ptr into the decoder instead of copying it, while preserving the existing slice and delta estimator arguments.
🤖 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 `@include/mzpeak/data/encoding.h`:
- Around line 153-156: Replace the bare string throws in the encoding.h
chunked-layout branch and both null-marking failure paths in null_marking.h with
structured exception types derived from std::exception. Use ParquetError for the
unimplemented chunked-layout path, including the affected dimension name in its
message, and use one consistent structured exception type for the two
null-marking errors while preserving their existing diagnostic context.
In `@include/mzpeak/data/null_marking.h`:
- Around line 90-123: Update Decoder<T>::chunk so the trailing range uses the
exclusive array length as its end: set range.end to array_->length() after the
index loop and before the final range.empty() check/push. Preserve the existing
range construction and null handling.
- Around line 126-189: In Decoder<T>::operator(), replace the range-size-based
delta scaling with the actual distance between the requested null index and its
anchor valid sample. Compute the index distance from index to range.end - 1 when
range.begin < index, or to range.begin otherwise, and apply the corresponding
signed adjustment to prior_.value while preserving prior_.delta estimation.
In `@include/mzpeak/util/algorithm.h`:
- Around line 42-52: Update deltas() to handle an empty values vector before
calculating the reserve size or iterating, returning an empty result directly.
Preserve the existing delta computation for non-empty inputs and avoid
values.size() - 1 underflow.
In `@test/spectra_test.cpp`:
- Around line 39-42: Correct the monotonicity assertion in the loop over mz so
each current value is compared with the preceding value rather than itself.
Update the BOOST_TEST expression within the visible range-based iteration,
preserving the existing nondecreasing-order requirement.
---
Outside diff comments:
In `@src/data/array_index.cpp`:
- Line 88: Update the entries_ sort in the array-index construction path to use
array_name as the primary key and name as a secondary key, ensuring entries
sharing an array_name have deterministic chunk ordering before grouping.
---
Nitpick comments:
In `@include/mzpeak/util/algorithm.h`:
- Around line 61-76: Remove the redundant std::ranges::sort call from
median_delta and rely on boost::math::statistics::median for the initial median
calculation. Preserve the existing delta filtering, empty-result handling, and
final median behavior.
In `@src/data/array_index.cpp`:
- Around line 19-25: Rename the misspelled local variable from_tansform to
from_transform within ArrayIndex::Dimension::needs_delta_model, updating its
declaration and return expression while preserving the existing logic.
In `@src/schema/psi/transform.cpp`:
- Around line 67-78: Update the fallback branch in the std::visit lambda within
the CV conversion flow to make the static assertion dependent on the template
type T, preventing unconditional evaluation on older compilers while retaining
the compile-time failure for unhandled variant alternatives.
In `@src/spectrum.cpp`:
- Around line 25-27: Update the decoder_ initialization in the Spectrum
constructor to move the by-value data shared_ptr into the decoder instead of
copying it, while preserving the existing slice and delta estimator arguments.
In `@test/array_index_test.cpp`:
- Around line 34-36: Update the construction of Schema::PSI::Transform from
transform_cv to use transform_cv.value() instead of dereferencing it with *.
Keep the existing BOOST_TEST assertion and surrounding test flow unchanged.
🪄 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: 0d0f9284-d5f6-41d4-8fc4-239c041ec6a9
📒 Files selected for processing (19)
include/mzpeak/data/array_index.hinclude/mzpeak/data/dimension.hinclude/mzpeak/data/encoding.hinclude/mzpeak/data/null_marking.hinclude/mzpeak/data/signals.hinclude/mzpeak/schema/psi/data_type.hinclude/mzpeak/schema/psi/transform.hinclude/mzpeak/spectrum.hinclude/mzpeak/util/algorithm.hinclude/mzpeak/util/decoders.hinclude/mzpeak/util/delta_estimator.hmeson.buildsrc/data/array_index.cppsrc/data/signals.cppsrc/schema/psi/transform.cppsrc/spectra.cppsrc/spectrum.cpptest/array_index_test.cpptest/spectra_test.cpp
💤 Files with no reviewable changes (1)
- include/mzpeak/data/dimension.h
Also, make the process of decoding signals abstract so that any class can use it.
|
@coderabbitai recheck. |
|
✅ Action performedReview finished.
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests