fix!: add serde derives for more types #112
Conversation
This is a breaking change because it changes the definition of some types so that they make more sense. Specifically, `OutlierOutput::outlying_series` is now a `BTreeSet` to preserve order, and `OutlierIntervals::intervals` is now a `Vec<OutlierInterval>` instead of a `Vec<usize>`, to make it more easily usable.
WalkthroughThe pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
crates/augurs-outlier/src/lib.rs (1)
273-293: Enhance test coverage for serializationIn the
serializetest, consider adding cases with intervals whereendisSome(usize)to ensure serialization handles both open and closed intervals correctly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- crates/augurs-core/src/distance.rs (1 hunks)
- crates/augurs-js/src/outlier.rs (4 hunks)
- crates/augurs-outlier/Cargo.toml (1 hunks)
- crates/augurs-outlier/src/dbscan.rs (3 hunks)
- crates/augurs-outlier/src/lib.rs (7 hunks)
- crates/augurs-outlier/src/mad.rs (2 hunks)
- crates/augurs-outlier/src/testing.rs (2 hunks)
Additional comments not posted (19)
crates/augurs-outlier/Cargo.toml (1)
27-28: LGTM!Adding
serde_jsonas a development dependency is a valid change. It expands the development environment by including a library for JSON serialization and deserialization, which can be useful for testing and development purposes. The specified version is a stable release, ensuring compatibility and reliability.This change does not affect the existing functionality of the crate and is limited to the development environment, making it a safe addition.
crates/augurs-core/src/distance.rs (1)
20-20: Conditional serialization support forDistanceMatrixstruct.The addition of the
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]attribute to theDistanceMatrixstruct is a great improvement. It allows instances ofDistanceMatrixto be serialized and deserialized when the "serde" feature is enabled, enhancing the struct's functionality for data interchange formats such as JSON.The conditional compilation ensures that the serde dependencies are only included when needed, keeping the dependencies minimal when serialization is not required. This provides flexibility to enable or disable serialization support based on the project's requirements.
crates/augurs-js/src/outlier.rs (4)
274-279: LGTM!The change simplifies the conversion logic by directly mapping
augurs_outlier::OutlierIntervalinstances toOutlierIntervalwithout the intermediate vector. This aligns with the AI-generated summary and improves the clarity of the code.
295-301: LGTM!The
Fromimplementation forOutlierIntervaldirectly mapsaugurs_outlier::OutlierIntervalinstances toOutlierInterval, streamlining the conversion process. This change aligns with the AI-generated summary and enhances the clarity and efficiency of the code by simplifying the conversion logic.
1-1: LGTM!The import of
BTreeSetfrom thestd::collectionsmodule aligns with the AI-generated summary, which mentions thatHashSethas been replaced withBTreeSetfor theoutlying_seriesfield in theOutlierOutputstruct. UsingBTreeSetis appropriate when the order of outlier indices is important, as it maintains its elements in sorted order.
310-310: Verify the impact of the change on the codebase.The change from
HashSet<usize>toBTreeSet<usize>for theoutlying_seriesfield in theOutlierOutputstruct aligns with the AI-generated summary. UsingBTreeSetis appropriate when the order of outlier indices is important, as it maintains its elements in sorted order.However, it's important to ensure that this change is consistently applied throughout the codebase and that any dependent code is updated accordingly, as the order of elements will now be preserved.
Run the following script to verify the usage of
outlying_seriesfield:Verification successful
The update to
BTreeSet<usize>foroutlying_seriesis consistently applied across the codebase. No issues detected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `outlying_series` field in the codebase. # Test: Search for the usage of `outlying_series` field. Expect: Consistent usage with `BTreeSet`. rg --type rust -A 5 $'outlying_series'Length of output: 4138
crates/augurs-outlier/src/testing.rs (1)
880-891: LGTM!The
flatten_intervalsfunction looks good. It correctly converts anOutlierIntervalsto a list of indices by iterating over the intervals, collecting thestartandendindices (if present), and flattening them into a single vector. The use ofiter(),flat_map(), andcollect()is idiomatic and efficient.crates/augurs-outlier/src/mad.rs (1)
Line range hint
386-716: LGTM!The changes in the test code, including the updated import statement and the flattening of intervals before comparison, appear to be a valid refactoring to accommodate the new structure of
outlier_intervals. The test coverage and correctness are maintained.crates/augurs-outlier/src/dbscan.rs (4)
360-360: LGTM!The import statement has been updated to include the
flatten_intervalsfunction from thetestingmodule, which is necessary to support the changes made in the test cases.
519-525: LGTM!The changes to the iteration over outlier indices simplify the logic and improve readability by using the flattened indices obtained from
flatten_intervals. The usage ofiterandnext()on the flattened indices is more idiomatic and consistent with the introduction offlatten_intervalsin the import statement.
602-606: LGTM!The assertions have been updated to retrieve indices from the flattened intervals, ensuring that they remain valid under the new implementation. The changes are consistent with the modifications made to the iteration logic.
608-614: LGTM!The assertions have been updated to use the flattened indices, ensuring that they access the correct values. The usage of array indexing is consistent with the flattened structure of the indices, and the assertions remain valid in testing the expected behavior of the outlier detection.
crates/augurs-outlier/src/lib.rs (7)
4-4: LGTM: ImportingBTreeSetto preserve orderThe import of
BTreeSetis appropriate for ensuring the order of outlying series is preserved, aligning with the PR objectives.
41-42: Enable serialization forOutlierOutputstructAdding
serdeserialization attributes allowsOutlierOutputto be serialized consistently, which is beneficial for data interchange.
45-47: Updateoutlying_seriesto useBTreeSetChanging
outlying_seriesto aBTreeSet<usize>ensures that the series indices are ordered, which can be important for downstream processing.
83-84: Enable serialization forSeriesstructAdding
serdeserialization attributes to theSeriesstruct facilitates consistent serialization of series results.
134-138: RefactorOutlierIntervalsto useintervalsand enable serializationReplacing
indiceswithintervals: Vec<OutlierInterval>provides a more structured and clear representation of outlier intervals. Serialization attributes enable consistent data exchange.
141-141: Exclude internal stateexpecting_endfrom serializationUsing
#[cfg_attr(feature = "serde", serde(skip))]appropriately omits the internalexpecting_endflag from serialized output, preventing exposure of internal logic.
182-196: AddOutlierIntervalstruct for clearer interval representationIntroducing
OutlierIntervalstruct enhances the clarity and usability of outlier intervals, allowing for explicit start and optional end indices.
|
|
||
| self.intervals.push(OutlierInterval { | ||
| start: ts, | ||
| end: None, | ||
| }); |
There was a problem hiding this comment.
Improve error handling in add_start method
The use of debug_assert! means the check is only active in debug builds. Consider using assert! or returning a Result to ensure that invalid states are caught in release builds as well.
|
|
||
| match self.intervals.last_mut() { | ||
| Some(x @ OutlierInterval { end: None, .. }) => { | ||
| x.end = Some(ts); | ||
| } | ||
| _ => unreachable!("tried to add end to an open-ended interval"), | ||
| }; |
There was a problem hiding this comment.
Improve error handling in add_end method
Similar to add_start, using debug_assert! and unreachable! may not catch errors in release builds. Consider explicit error handling to prevent potential panics in production.
This is a breaking change because it changes the definition of some types
so that they make more sense.
Specifically,
OutlierOutput::outlying_seriesis now aBTreeSetto preserveorder, and
OutlierIntervals::intervalsis now aVec<OutlierInterval>insteadof a
Vec<usize>, to make it more easily usable.Summary by CodeRabbit
New Features
DistanceMatrixstruct with conditional serialization and deserialization support.OutlierIntervalstruct for better representation of outlier intervals.flatten_intervalsfunction for streamlined processing of outlier intervals.Improvements
HashSetwithBTreeSetfor ordered outlier series.Documentation
serde_jsonas a development dependency for improved JSON handling in the development environment.