Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: define lints in Cargo.toml instead of each crate's lib.rs #138

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

sd2k
Copy link
Collaborator

@sd2k sd2k commented Oct 22, 2024

Summary by CodeRabbit

  • New Features

    • Introduced a Median Absolute Deviation (MAD) outlier detection mechanism with new structures and methods for enhanced outlier detection capabilities.
  • Bug Fixes

    • Adjusted visibility of several structs and methods to improve encapsulation and restrict access as needed.
  • Documentation

    • Removed various linting warnings related to documentation enforcement across multiple crates, which may affect code quality checks.
  • Chores

    • Added new linting configurations to several Cargo.toml files, enabling workspace-level linting rules.

Copy link
Contributor

coderabbitai bot commented Oct 22, 2024

Walkthrough

The changes involve updates to multiple Cargo.toml files across various crates, introducing a new [lints] section with workspace = true to enable workspace-level linting. Additionally, several Rust source files have had #![warn(...)] attributes removed, which previously enforced warnings for documentation and idiomatic practices. New structures and methods for outlier detection based on Median Absolute Deviation (MAD) have been added, enhancing functionality. Other modifications include visibility adjustments for specific structs and methods, primarily restricting access to the current crate.

Changes

File Path Change Summary
Cargo.toml (root) Added [workspace.lints.rust] with various lint rules set to "warn".
crates/augurs-changepoint/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-changepoint/src/lib.rs Added DefaultArgpcpDetectorBuilder struct and associated methods for building DefaultArgpcpDetector.
crates/augurs-clustering/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-clustering/src/lib.rs Removed #![warn(...)] attributes for documentation and idiomatic practices.
crates/augurs-core/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-core/src/lib.rs Removed #![warn(...)] attributes; added ModelError trait for error handling.
crates/augurs-dtw/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-dtw/src/lib.rs Removed #![warn(...)] attributes; core functionality remains unchanged.
crates/augurs-ets/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-ets/src/ets.rs Changed visibility of FitState and Ets structs and methods to pub(crate).
crates/augurs-ets/src/lib.rs Added Error enum and type alias Result<T>.
crates/augurs-ets/src/model.rs Changed visibility of n_included method to pub(crate); simplified lifetime handling in Forecast.
crates/augurs-ets/src/stat.rs Changed visibility of VarExt trait to pub(crate).
crates/augurs-forecaster/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-forecaster/src/lib.rs Removed #![warn(...)] attributes.
crates/augurs-js/Cargo.toml Added [lints] section with workspace = true; updated wasm-pack.profile.release options.
crates/augurs-js/src/lib.rs Removed #![warn(...)] attributes; core functionality remains unchanged.
crates/augurs-mstl/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-mstl/src/lib.rs No changes to exported or public entities.
crates/augurs-outlier/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-outlier/src/dbscan.rs Changed visibility of DBScan1DResults struct to pub(crate); modified static variable visibility.
crates/augurs-outlier/src/lib.rs Removed #![warn(missing_docs)] attribute.
crates/augurs-outlier/src/mad.rs Added MAD detection structures and methods; enhanced functionality.
crates/augurs-outlier/src/sensitivity.rs Changed visibility of Sensitivity struct to pub(crate).
crates/augurs-outlier/src/testing.rs Changed visibility of SERIES constant and flatten_intervals function to pub(crate).
crates/augurs-prophet/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-prophet/build.rs Introduced build script for compiling the Prophet model with error handling and temporary file management.
crates/augurs-prophet/src/lib.rs Removed #![warn(...)] attribute.
crates/augurs-seasons/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-seasons/src/lib.rs Removed #![warn(...)] attributes.
crates/augurs-testing/Cargo.toml Added [lints] section with workspace = true.
crates/augurs-testing/src/lib.rs Removed #![warn(...)] attributes.
crates/augurs/Cargo.toml Added [lints] section with workspace = true.
crates/augurs/src/lib.rs Removed #![warn(...)] attribute.
crates/pyaugurs/Cargo.toml Added [lints] section with workspace = true.
crates/pyaugurs/src/lib.rs Removed #![warn(...)] attributes; core functionality remains unchanged.

Poem

In the garden of code where the rabbits play,
New lints hop in, guiding the way.
With builders and structs, oh what a delight,
Outliers detected, our future looks bright!
So let’s dance through the changes, with joy and with cheer,
For in this code burrow, we’ve nothing to fear! 🐇✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sd2k sd2k enabled auto-merge (squash) October 22, 2024 11:16
@sd2k sd2k merged commit 57f4d6b into main Oct 22, 2024
21 checks passed
@sd2k sd2k deleted the workspace-lints branch October 22, 2024 11:21
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (17)
crates/augurs-testing/Cargo.toml (2)

20-21: LGTM: Centralized lint configuration added.

The addition of the [lints] section with workspace = true is a positive change. This modification centralizes lint configurations at the workspace level, which can lead to more consistent code quality across the entire project.

This change promotes better maintainability by:

  1. Ensuring uniform linting rules across all crates in the workspace.
  2. Simplifying future updates to linting rules, as they can be managed in one place.
  3. Reducing the need for redundant lint configurations in individual crates.

Consider documenting this change in the project's contribution guidelines to ensure all developers are aware of the centralized linting approach.


Line range hint 1-21: Consider using workspace dependencies for common crates.

The current Cargo.toml file looks good, but there's an opportunity for further improvement in dependency management.

Consider moving common dependencies like assert_approx_eq and once_cell to the workspace's Cargo.toml file if they are used across multiple crates. This can be done by adding a [workspace.dependencies] section in the root Cargo.toml and then referencing them in this file like so:

[dependencies]
assert_approx_eq = { workspace = true }
once_cell = { workspace = true }

This approach can help maintain consistent versions across the workspace and simplify dependency updates.

crates/augurs-clustering/Cargo.toml (1)

25-27: LGTM! Consider adding a comment for clarity.

The addition of the [lints] section with workspace = true is correct and aligns with the PR objective of defining lints at the workspace level. This change will help maintain consistent linting across the project.

Consider adding a brief comment above the [lints] section to explain its purpose, for example:

# Use workspace-level linting configuration
[lints]
workspace = true

This can help other developers understand the reason for this section at a glance.

crates/augurs/Cargo.toml (1)

47-48: Approve: Centralized lint configuration

The addition of the [lints] section with workspace = true is a positive change that aligns well with the PR objectives. This modification centralizes lint configurations at the workspace level, which offers several benefits:

  1. Ensures consistency in linting rules across all packages in the workspace.
  2. Simplifies maintenance by providing a single point of configuration for lints.
  3. Aligns with best practices for managing large Rust projects with multiple crates.

Consider documenting this change in the project's README or contributing guidelines to ensure all contributors are aware of the new linting strategy.

Cargo.toml (1)

60-67: Excellent addition of workspace-level lints!

The introduction of workspace-level lints in the Cargo.toml file is a great improvement. It centralizes lint configurations, promoting consistency across the project. The chosen lints (missing_debug_implementations, missing_docs, rust_2018_idioms, and unreachable_pub) will enhance code quality, documentation, and adherence to Rust best practices.

The decision to allow non_local_definitions with a justification and link to the relevant Rust issue (rust-lang/rust#131643) is well-documented.

Consider adding a TODO comment to revisit the non_local_definitions lint once the linked issue is resolved. This will help ensure it's not overlooked in the future.

 # Allow `non_local_definitions` until https://github.com/rust-lang/rust/issues/131643
 # is resolved.
+# TODO: Revisit this configuration once the above issue is closed.
 non_local_definitions = { level = "allow", priority = 1 }
crates/augurs-prophet/build.rs (3)

Line range hint 13-14: Incorrect use of .parse() when converting STAN_PATH to PathBuf

The .parse() method is not suitable for converting a String to a PathBuf because PathBuf does not implement the FromStr trait. Instead, use PathBuf::from to directly construct a PathBuf from the String returned by std::env::var("STAN_PATH"). This change will prevent a potential runtime error when the build script attempts to parse the environment variable.

Apply this diff to fix the issue:

-let stan_path: PathBuf = std::env::var("STAN_PATH")
-    .map_err(|_| "STAN_PATH not set")?
-    .parse()
-    .map_err(|_| "invalid STAN_PATH")?;
+let stan_path = PathBuf::from(std::env::var("STAN_PATH")
+    .map_err(|_| "STAN_PATH not set")?);

Line range hint 79-80: Rename _result to result for clarity

In Rust, a leading underscore in a variable name indicates that the variable may be unused. Since _result is actively used later in the code, consider renaming it to result without the leading underscore to improve code clarity and adhere to Rust naming conventions.

Apply this diff:

-let _result = Ok::<(), &'static str>(());
+let result = Ok::<(), &'static str>(());
 #[cfg(all(feature = "cmdstan", feature = "compile-cmdstan"))]
-let _result = compile_model();
+let result = compile_model();

Also, update subsequent references to _result to result throughout the main function.


Line range hint 81-94: Refactor the conditional logic in main for better readability

The current conditional logic in the main function is complex due to multiple #[cfg(...)] attributes and environment variable checks. This complexity can make the code harder to maintain and understand. Consider refactoring the main function to simplify the control flow. For example, you can:

  • Consolidate feature flags and environment variable conditions into a single control flow structure.
  • Extract repeated or similar logic into helper functions.
  • Use explicit condition branches to make the execution paths clearer.
crates/augurs-changepoint/src/lib.rs (1)

Line range hint 156-159: Enhance the builder by adding setter methods for all fields

Currently, the DefaultArgpcpDetectorBuilder only provides a method to set constant_value. To fully leverage the builder pattern and offer flexibility to users, consider adding setter methods for the other fields: length_scale, noise_level, max_lag, alpha0, beta0, logistic_hazard_h, logistic_hazard_a, and logistic_hazard_b.

Here's a suggestion to add setter methods for the remaining fields:

 impl DefaultArgpcpDetectorBuilder {
     /// Set the value for the constant kernel.
     pub fn constant_value(mut self, cv: f64) -> Self {
         self.constant_value = cv;
         self
     }
+    /// Set the length scale for the RBF kernel.
+    pub fn length_scale(mut self, ls: f64) -> Self {
+        self.length_scale = ls;
+        self
+    }
+
+    /// Set the noise level for the White kernel.
+    pub fn noise_level(mut self, nl: f64) -> Self {
+        self.noise_level = nl;
+        self
+    }
+
+    /// Set the maximum lag.
+    pub fn max_lag(mut self, lag: NonZeroUsize) -> Self {
+        self.max_lag = lag;
+        self
+    }
+
+    /// Set the alpha0 parameter.
+    pub fn alpha0(mut self, alpha: f64) -> Self {
+        self.alpha0 = alpha;
+        self
+    }
+
+    /// Set the beta0 parameter.
+    pub fn beta0(mut self, beta: f64) -> Self {
+        self.beta0 = beta;
+        self
+    }
+
+    /// Set the logistic hazard h parameter.
+    pub fn logistic_hazard_h(mut self, h: f64) -> Self {
+        self.logistic_hazard_h = h;
+        self
+    }
+
+    /// Set the logistic hazard a parameter.
+    pub fn logistic_hazard_a(mut self, a: f64) -> Self {
+        self.logistic_hazard_a = a;
+        self
+    }
+
+    /// Set the logistic hazard b parameter.
+    pub fn logistic_hazard_b(mut self, b: f64) -> Self {
+        self.logistic_hazard_b = b;
+        self
+    }
 }
crates/augurs-outlier/src/mad.rs (8)

Line range hint 1-1: Remove outdated TODO comment

The comment // TODO: add MAD implementation. is no longer relevant since the MAD implementation has been added to this file. Consider removing this comment to keep the codebase clean and up-to-date.


Line range hint 2-2: Clean up commented-out attribute

The line // #![allow(dead_code, unused_variables)] is a commented-out attribute. If it's no longer necessary, consider removing it to reduce clutter in the code.


Line range hint 359-371: Fix potential off-by-one error in slicing and weights calculation

There is a potential off-by-one error in the thd_quantile function when slicing x and calculating weights:

  • In line 359, weights is calculated over the range (left_index..(right_index + 1)), which includes right_index.
  • In line 367, x is sliced as x[left_index..right_index], which excludes right_index.

This mismatch can lead to a difference in lengths between weights and x[left_index..right_index], possibly causing panics or incorrect calculations.

To fix this issue, adjust the slicing of x to include right_index by changing it to x[left_index..=right_index].

Apply this diff to correct the slicing:

-     Ok(x[left_index..right_index]
+     Ok(x[left_index..=right_index]
          .iter()
          .zip(weights)
          .map(|(&x_i, w)| x_i * w)
          .sum())

Line range hint 136-147: Propagate specific errors from median calculations

In the calculate_double_medians function, when calling thd_median, if an error occurs, it's currently being converted into a generic DivideByZero error:

if let (Ok(lower), Ok(upper)) = (lower, upper) {
    if lower == 0.0 || upper == 0.0 {
        Err(PreprocessingError::from(MADError::DivideByZero))
    } else {
        // ...
    }
} else {
    Err(PreprocessingError::from(MADError::DivideByZero))
}

Consider propagating the specific errors returned by thd_median to provide more detailed error information.

Apply this diff to propagate the original errors:

- if let (Ok(lower), Ok(upper)) = (lower, upper) {
+ let lower = lower.map_err(|e| PreprocessingError::from(e))?;
+ let upper = upper.map_err(|e| PreprocessingError::from(e))?;
    if lower == 0.0 || upper == 0.0 {
        Err(PreprocessingError::from(MADError::DivideByZero))
    } else {
        // ...
    }
- } else {
-     Err(PreprocessingError::from(MADError::DivideByZero))
- }

Line range hint 175-177: Handle infinite scores with clearer logic

In the calculate_mad function, you're setting score to NaN if it becomes infinite:

if score.is_infinite() {
    score = f64::NAN;
}

Consider handling this case more explicitly or logging a warning, as infinite scores may indicate issues with the input data or calculation.


Line range hint 51-54: Consider making Medians fields public or adding accessor methods

The Medians struct is public, but its fields are private:

pub struct Medians {
    lower: f64,
    global: f64,
    upper: f64,
}

If there is a need to access these fields outside the crate, consider making them public or providing public accessor methods.


Line range hint 252-255: Review the visibility of PreprocessedData fields

Similarly, the PreprocessedData struct is public with private fields:

pub struct PreprocessedData {
    medians: Medians,
    mad_scores: Vec<Vec<f64>>,
}

Evaluate whether the fields should be accessible outside the crate. If necessary, provide public methods to access them or adjust their visibility.


Line range hint 315-316: Review convergence criteria in beta_hdi

In the beta_hdi function, you're using f64::EPSILON for the convergence epsilon:

let mut convergency = SimpleConvergency {
    eps: f64::EPSILON,
    max_iter: 30,
};

f64::EPSILON is a very small number (2.220446049250313e-16) and might be unnecessarily precise, potentially causing the root-finding algorithm to fail to converge within the maximum iterations. Consider using a slightly larger epsilon, such as 1e-9 or a configurable parameter.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a0e0820 and d72d182.

📒 Files selected for processing (37)
  • Cargo.toml (1 hunks)
  • crates/augurs-changepoint/Cargo.toml (1 hunks)
  • crates/augurs-changepoint/src/lib.rs (1 hunks)
  • crates/augurs-clustering/Cargo.toml (1 hunks)
  • crates/augurs-clustering/src/lib.rs (0 hunks)
  • crates/augurs-core/Cargo.toml (1 hunks)
  • crates/augurs-core/src/lib.rs (0 hunks)
  • crates/augurs-dtw/Cargo.toml (1 hunks)
  • crates/augurs-dtw/src/lib.rs (0 hunks)
  • crates/augurs-ets/Cargo.toml (1 hunks)
  • crates/augurs-ets/src/ets.rs (3 hunks)
  • crates/augurs-ets/src/lib.rs (0 hunks)
  • crates/augurs-ets/src/model.rs (2 hunks)
  • crates/augurs-ets/src/stat.rs (1 hunks)
  • crates/augurs-forecaster/Cargo.toml (1 hunks)
  • crates/augurs-forecaster/src/lib.rs (0 hunks)
  • crates/augurs-js/Cargo.toml (1 hunks)
  • crates/augurs-js/src/lib.rs (0 hunks)
  • crates/augurs-mstl/Cargo.toml (1 hunks)
  • crates/augurs-mstl/src/lib.rs (0 hunks)
  • crates/augurs-outlier/Cargo.toml (1 hunks)
  • crates/augurs-outlier/src/dbscan.rs (2 hunks)
  • crates/augurs-outlier/src/lib.rs (0 hunks)
  • crates/augurs-outlier/src/mad.rs (2 hunks)
  • crates/augurs-outlier/src/sensitivity.rs (1 hunks)
  • crates/augurs-outlier/src/testing.rs (1 hunks)
  • crates/augurs-prophet/Cargo.toml (1 hunks)
  • crates/augurs-prophet/build.rs (1 hunks)
  • crates/augurs-prophet/src/lib.rs (0 hunks)
  • crates/augurs-seasons/Cargo.toml (1 hunks)
  • crates/augurs-seasons/src/lib.rs (0 hunks)
  • crates/augurs-testing/Cargo.toml (1 hunks)
  • crates/augurs-testing/src/lib.rs (0 hunks)
  • crates/augurs/Cargo.toml (1 hunks)
  • crates/augurs/src/lib.rs (0 hunks)
  • crates/pyaugurs/Cargo.toml (1 hunks)
  • crates/pyaugurs/src/lib.rs (0 hunks)
💤 Files with no reviewable changes (13)
  • crates/augurs-clustering/src/lib.rs
  • crates/augurs-core/src/lib.rs
  • crates/augurs-dtw/src/lib.rs
  • crates/augurs-ets/src/lib.rs
  • crates/augurs-forecaster/src/lib.rs
  • crates/augurs-js/src/lib.rs
  • crates/augurs-mstl/src/lib.rs
  • crates/augurs-outlier/src/lib.rs
  • crates/augurs-prophet/src/lib.rs
  • crates/augurs-seasons/src/lib.rs
  • crates/augurs-testing/src/lib.rs
  • crates/augurs/src/lib.rs
  • crates/pyaugurs/src/lib.rs
✅ Files skipped from review due to trivial changes (4)
  • crates/augurs-changepoint/Cargo.toml
  • crates/augurs-mstl/Cargo.toml
  • crates/augurs-seasons/Cargo.toml
  • crates/pyaugurs/Cargo.toml
🧰 Additional context used
🔇 Additional comments (25)
crates/augurs-core/Cargo.toml (1)

17-19: Approve the addition of workspace-level linting.

The introduction of the [lints] section with workspace = true is a positive change. This modification centralizes lint configuration at the workspace level, which can lead to more consistent code quality across all crates in the project. It aligns well with the PR objective of defining lints in Cargo.toml instead of each crate's lib.rs.

crates/augurs-forecaster/Cargo.toml (1)

23-25: LGTM! Centralized lint configuration added.

The addition of the [lints] section with workspace = true aligns with the PR objective of defining lints in Cargo.toml instead of each crate's lib.rs. This change promotes consistency across the project and simplifies lint rule maintenance.

To ensure this change is part of a consistent workspace-wide update, please run the following script:

This script will help verify that:

  1. All Cargo.toml files in the workspace have the [lints] section with workspace = true.
  2. The corresponding #![warn(...)] attributes have been removed from lib.rs files.
crates/augurs-dtw/Cargo.toml (1)

31-33: LGTM! Workspace-level linting configuration added.

The addition of the [lints] section with workspace = true is a positive change. This modification aligns with the PR objective of defining lints in Cargo.toml instead of each crate's lib.rs. It will lead to more consistent linting across the entire workspace, potentially improving code quality and maintainability.

crates/augurs-outlier/Cargo.toml (1)

34-35: Approve the addition of workspace-level lints.

The introduction of the [lints] section with workspace = true is a positive change. This approach centralizes lint configurations at the workspace level, which offers several benefits:

  1. Ensures consistency in code quality checks across different packages in the workspace.
  2. Simplifies maintenance by managing lint rules in one place.
  3. Makes it easier to enforce project-wide coding standards.

This change aligns well with best practices for managing large Rust projects with multiple crates.

crates/augurs-ets/Cargo.toml (1)

47-48: LGTM! Workspace-level linting configuration added.

The addition of the [lints] section with workspace = true is a good change. This modification centralizes linting configuration at the workspace level, which can lead to more consistent code quality across the project. It aligns well with the PR objective of defining lints in Cargo.toml instead of in each crate's lib.rs file.

crates/augurs-ets/src/stat.rs (3)

Line range hint 2-33: Implementation LGTM.

The implementation of the VarExt trait is efficient and well-structured. It uses a single-pass algorithm for calculating variance, which is a good practice for performance. The code is clear and easy to understand.


Line range hint 35-47: Test coverage looks good.

The test module provides good coverage for the var method, testing both cases with and without Bessel's correction. The use of assert_eq! for exact comparison and assert_approx_eq! for approximate comparison is appropriate.


1-1: Approve visibility change, verify impact.

The change from pub to pub(crate) for the VarExt trait is a good practice as it limits the public API surface area. This aligns with the principle of encapsulation and information hiding.

To ensure this change doesn't break any existing code, please run the following script to check for any usage of VarExt outside of this crate:

If the script returns any results, those usages will need to be updated or the visibility change reconsidered.

crates/augurs-outlier/src/sensitivity.rs (2)

Line range hint 1-1: Workspace-level linting configuration added.

The addition of [lints] workspace = true in Cargo.toml enables workspace-level linting. This change promotes consistent code quality across the entire project. Consider the following:

  1. Ensure that all crates in the workspace are compatible with the new linting rules.
  2. Review and remove any redundant crate-specific lint configurations.
  3. Update the project documentation to reflect this change in the linting strategy.

To check for crate-specific lint configurations that might now be redundant:

#!/bin/bash
# Search for lint attributes in Rust files
rg -p '#!\[.*lint' --type rust

# Search for [lints] sections in Cargo.toml files
rg -p '\[lints\]' --type toml

16-16: Visibility change enhances encapsulation, but verify impact on dependent code.

The change from pub to pub(crate) for the Sensitivity struct improves encapsulation by restricting its visibility to the current crate. This aligns with good software design principles. However, consider the following:

  1. Verify that this change doesn't break any existing code that depends on Sensitivity being public.
  2. The internal f64 field is still pub. Consider making it private for complete encapsulation.

To check for potential breakage, run:

✅ Verification successful

Visibility change verified; no external usages found.

  • The Sensitivity struct is not used outside the augurs-outlier crate.
  • Consider making the internal f64 field private for complete encapsulation.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of Sensitivity outside the current crate
rg -p "use .*Sensitivity" --glob '!crates/augurs-outlier/**'

Length of output: 670


Script:

#!/bin/bash
# Search for any references to 'Sensitivity' outside the 'augurs-outlier' crate
rg -w 'Sensitivity' --glob '!crates/augurs-outlier/**' --type rust

Length of output: 351

crates/augurs-js/Cargo.toml (1)

52-53: Lint configuration aligned with workspace.

The addition of the [lints] section with workspace = true aligns with the PR objective of defining lints at the workspace level instead of in each crate's lib.rs. This change promotes consistency and easier maintenance of linting rules across the project.

Let's verify if this change has been consistently applied across all crates in the workspace:

#!/bin/bash
# Description: Check for consistent lint configuration across the workspace

# Test: Search for [lints] sections in all Cargo.toml files
# Expect: Consistent use of 'workspace = true' in all crates
rg -U --type toml '\[lints\](\n|\r\n)workspace = true' crates/*/Cargo.toml
crates/augurs-prophet/Cargo.toml (1)

53-54: LGTM: Workspace-level lint configuration added.

The addition of the [lints] section with workspace = true is a positive change. This modification centralizes lint configurations at the workspace level, which can lead to more consistent code quality across the project. It aligns well with similar updates in other packages, as mentioned in the PR summary.

crates/augurs-ets/src/ets.rs (5)

10-10: Approved: Improved encapsulation for FitState struct

The visibility change from pub to pub(crate) for the FitState struct enhances encapsulation by restricting access to the current crate only. This is a good practice for internal structures that should not be directly accessed or modified by external crates.


32-32: Approved: Consistent visibility changes for FitState methods

The visibility of all FitState methods has been changed from pub to pub(crate), which is consistent with the struct's visibility change. This maintains a coherent API design and further reinforces the encapsulation of the FitState implementation within the crate.

Also applies to: 38-38, 46-46, 51-51, 58-58, 66-66, 74-74, 80-80, 86-86, 92-92, 98-98


108-108: Approved: Enhanced encapsulation for Ets struct

The visibility of the Ets struct has been changed from pub to pub(crate), which is consistent with the changes made to FitState. This change further reinforces the encapsulation of the ETS implementation within the crate, preventing direct access from external crates.


Line range hint 169-169: Approved: Consistent visibility changes for Ets methods

The visibility of etscalc_in and pegels_resid_in methods has been changed from pub to pub(crate), which is consistent with the Ets struct's visibility change. This further restricts access to internal implementation details, enhancing the overall encapsulation of the ETS functionality within the crate.

Also applies to: 324-324


Line range hint 1-724: Summary: Improved encapsulation and crate-level visibility

The changes in this file consistently improve encapsulation by changing the visibility of FitState and Ets structs, along with their associated methods, from pub to pub(crate). This aligns with the PR objective of defining lints at the workspace level and restricts direct access to these components from external crates. The internal logic, error handling, and control flow remain unchanged, focusing solely on visibility modifications to enhance the module's API design.

crates/augurs-outlier/src/testing.rs (2)

3-3: Approved: Visibility restriction enhances encapsulation.

The change from pub to pub(crate) for SERIES is a good practice. It restricts access to this test data within the crate, preventing unnecessary exposure to external crates and improving encapsulation.


Line range hint 307-317: Approved: Consistent visibility restriction improves API design.

The change from pub to pub(crate) for flatten_intervals is appropriate. It aligns with the visibility change of SERIES and indicates that this function is intended for internal use within the crate. This change helps in defining a clearer public API boundary for the crate.

crates/augurs-outlier/src/dbscan.rs (2)

Line range hint 258-262: Approve visibility change for DBScan1DResults

The change from pub to pub(crate) for DBScan1DResults is a good practice. It restricts the struct's visibility to the current crate, which helps in encapsulating internal components and prevents unintended usage outside the crate.


Line range hint 371-386: Approve visibility change for DBSCAN_DATASET

Changing the visibility of DBSCAN_DATASET from pub to static is appropriate. This dataset is used only for testing purposes within this module, and limiting its accessibility helps prevent unintended usage elsewhere.

crates/augurs-ets/src/model.rs (2)

302-304: Visibility change improves encapsulation

The visibility of the n_included method has been changed from pub to pub(crate). This change restricts the method's accessibility to within the current crate, which improves encapsulation and reduces the public API surface. The function logic remains correct and efficient.


1228-1228: Simplified lifetime syntax

The implementation block for Forecast now uses the '_ syntax for lifetimes instead of an explicit 'a. This change simplifies the code without altering its functionality. It leverages Rust's lifetime elision rules, allowing the compiler to infer the appropriate lifetime.

crates/augurs-outlier/src/mad.rs (2)

Line range hint 87-87: Ensure proper handling of sensitivity conversion

In the with_sensitivity method, you're using sensitivity.try_into()? to convert an f64 into Sensitivity. Ensure that the TryFrom implementation for Sensitivity properly validates the input range (typically between 0 and 1) and provides meaningful error messages if the conversion fails.


Line range hint 292-327: Add error handling for edge cases in beta_hdi

The beta_hdi function may encounter edge cases where the calculated left and right values do not meet expected conditions, especially with extreme alpha, beta, or width values. Ensure that all possible scenarios are accounted for and that meaningful errors are returned when necessary.

Comment on lines 49 to 50
# previously had just ['-O4']
wasm-opt = ['-O4', '--enable-bulk-memory', '--enable-threads']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistent wasm-opt flags across Cargo.toml files.

Only crates/augurs-js/Cargo.toml includes the updated wasm-opt flags (['-O4', '--enable-bulk-memory', '--enable-threads']). The following crates are missing these configurations:

  • crates/augurs-changepoint/Cargo.toml
  • crates/augurs-clustering/Cargo.toml
  • crates/augurs-core/Cargo.toml
  • crates/augurs-dtw/Cargo.toml
  • crates/augurs-ets/Cargo.toml
  • crates/augurs-forecaster/Cargo.toml
  • crates/augurs-mstl/Cargo.toml
  • crates/augurs-outlier/Cargo.toml
  • crates/augurs-prophet/Cargo.toml
  • crates/augurs-seasons/Cargo.toml
  • crates/augurs-testing/Cargo.toml
  • crates/augurs/Cargo.toml
  • crates/pyaugurs/Cargo.toml

Please update these Cargo.toml files to include the new wasm-opt flags to ensure consistent WebAssembly optimizations across the workspace.

🔗 Analysis chain

Improved WebAssembly optimization configuration.

The addition of --enable-bulk-memory and --enable-threads flags to the wasm-opt configuration is a good improvement. These flags can potentially enhance the performance of the WebAssembly output.

  • --enable-bulk-memory allows for more efficient memory operations.
  • --enable-threads enables support for multi-threading in WebAssembly.

To ensure consistency across the workspace, let's verify if similar changes have been made in other relevant Cargo.toml files:


🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent wasm-opt flags across the workspace

# Test: Search for wasm-opt configurations in all Cargo.toml files
# Expect: Consistent use of new flags in relevant crates
rg -U --type toml 'wasm-opt.*\[.*\]' crates/*/Cargo.toml

Length of output: 1723


Script:

#!/bin/bash
# Description: Verify consistency of wasm-opt flags across the workspace

# Test: Search for wasm-opt configurations in all Cargo.toml files
# Expect: All relevant Cargo.toml files contain the updated wasm-opt flags

rg -U --type toml '\bwasm-opt\s*=\s*\[.*\]' crates/*/Cargo.toml

Length of output: 575

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant