feat!: split JS package into separate crates #149
Conversation
This enables us to package up each piece of functionality as a separate WASM module, which means users can import only the functionality they need to save on bundle size. It doesn't work as-is because some types are expected to work across packages but they're technically considered distinct types after compilation. It's possible that doing this using WASM components and JCO will achieve this but I'm not certain.
|
Warning Rate limit exceeded@sd2k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant structural changes to the JavaScript bindings for the Changes
Possibly related issues
Possibly related PRs
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 (
|
It complicates the build process because we need to use a nightly toolchain with a custom std library build, and complicates the bundler process for users too.
Before this commit we were using a shared DistanceMatrix type from augurs-core-js, and assuming that we could return it from one WASM module and use it directly in a second. This doesn't work, however; those are separate types at runtime and the memory is not shared between the two WASM modules. Instead, just return an array of Float64Arrays when calculating the distance matrix and accept that in the clustering side.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (38)
js/testpkg/seasons.test.ts (2)
3-3: Consider making the WASM module path resolution more robust.While the changes align with the PR's objective of splitting packages, the hardcoded path to the WASM module might cause issues in different environments or build setups.
Consider using dynamic import or URL resolution:
-initSync({ module: readFileSync('node_modules/@bsull/augurs/seasons_bg.wasm') }); +import.meta.resolve?.('@bsull/augurs/seasons_bg.wasm').then(url => { + initSync({ module: readFileSync(new URL(url)) }); +});Also applies to: 7-7
Line range hint
31-37: Improve test clarity and reduce duplication.The test cases are identical in both description and assertion, only differing in input type. This violates the DRY principle and reduces test clarity.
Consider consolidating the tests:
- it('works with number arrays', () => { - expect(seasonalities(y)).toEqual(new Uint32Array([4])); - }); - - it('works with number arrays', () => { - expect(seasonalities(new Float64Array(y))).toEqual(new Uint32Array([4])); - }); + it.each([ + ['regular array', y], + ['Float64Array', new Float64Array(y)] + ])('detects seasonal pattern with period 4 in %s', (_, input) => { + expect(seasonalities(input)).toEqual(new Uint32Array([4])); + });js/augurs-outlier-js/Cargo.toml (1)
13-18: Consider maintaining documentation despite WASM targetWhile the library configuration is correct for WASM compilation, consider enabling
doc = trueto maintain API documentation for developers, even if it's not included in the final WASM bundle.js/testpkg/logging.test.ts (1)
Line range hint
9-24: Consider adding tests for cross-package interactions.The existing tests cover basic initialization scenarios well. However, given the package split, consider adding tests to verify:
- Interaction between different WASM modules when used together
- Error handling when required WASM modules are missing
- Initialization order dependencies between split packages
Would you like me to help generate additional test cases for these scenarios?
js/augurs-core-js/Cargo.toml (1)
Line range hint
41-43: Consider documenting WASM optimization flags.The addition of
--enable-bulk-memoryand--enable-threadsflags is significant:
bulk-memory: Enables more efficient memory operationsthreads: Adds support for threading featuresConsider adding a comment explaining why these flags are necessary for the split architecture.
[package.metadata.wasm-pack.profile.release] -# previously had just ['-O4'] +# Enhanced optimization flags for better performance: +# - enable-bulk-memory: Enables efficient memory operations +# - enable-threads: Adds threading support for concurrent operations wasm-opt = ['-O4', '--enable-bulk-memory', '--enable-threads']js/justfile (2)
13-23: Consider making the npm scope and output directory configurable.The build configuration has hardcoded values that might need to change:
- The npm scope
@bsullshould be configurable for different organizations- The output directory
../augursis hardcoded which could make it less flexibleConsider this improvement:
-build-inner target args='': +build-inner target args='' scope='bsull' outdir='augurs': cd augurs-{{target}}-js && \ rm -rf ./pkg && \ wasm-pack build \ - --scope bsull \ + --scope {{scope}} \ --out-name {{target}} \ --release \ --target web \ --no-pack \ - --out-dir ../augurs \ + --out-dir ../{{outdir}} \ -- {{args}}
33-37: Consider adding test coverage requirements.The test sequence is good but could be enhanced with:
- Test coverage thresholds
- Coverage reporting to track changes over time
Consider adding coverage configuration to your test command:
test: cd testpkg && \ npm ci && \ npm run typecheck && \ - npm run test:ci + npm run test:ci -- --coverage --coverageThreshold='{"global":{"branches":80,"functions":80,"lines":80}}'js/augurs-dtw-js/Cargo.toml (3)
20-25: Enhance parallel feature documentation.The parallel feature documentation could be improved by:
- Adding a code example showing how to enable the feature
- Documenting the performance implications
- Explaining the browser compatibility requirements (threads support)
27-35: Consider using workspace version for wasm-bindgen-rayon.The
wasm-bindgen-rayondependency is pinned to version "1.2.1" while other dependencies use workspace versions. Consider moving this version specification to the workspace root to maintain consistent version management across all packages.
1-42: Document the modular WASM architecture.This package structure aligns well with the goal of enabling individual WASM modules. To improve maintainability and user experience, consider:
- Adding a README.md explaining the modular architecture
- Documenting how users can optimize their bundle size by importing specific features
- Providing examples of different import combinations and their resulting bundle sizes
Would you like me to help create a template for this documentation?
js/testpkg/ets.test.ts (1)
Line range hint
11-52: Consider adding cross-package integration tests.While the existing tests comprehensively cover ETS functionality, the PR objectives mention issues with types operating across different packages. Consider adding test cases that verify type compatibility when using ETS with other augurs packages.
Example test scenario:
it('maintains type compatibility with other augurs packages', async () => { // Import from different packages const etsResult = new AutoETS(5, 'ZZZ').fit(y); // Verify type compatibility when passing results between packages // Example: passing ETS results to another augurs package });justfile (1)
Line range hint
1-58: Consider WASM component model for type sharing.Given the PR objective mentioning issues with types across packages, consider exploring the WASM component model and JCO (JavaScript Component Object) for better type sharing between packages. This could help resolve the type compatibility issues while maintaining the benefits of separate packages.
Some considerations:
- WASM components could provide a standardized way to share types
- JCO could help with JavaScript interop
- This might require adjustments to the build process in the future
js/testpkg/clustering.test.ts (1)
8-9: Consider making WASM module paths configurable.The hardcoded paths to WASM modules in node_modules might be brittle across different environments or build configurations.
Consider extracting these paths to a configuration file or environment variables:
-initClusteringSync({ module: readFileSync('node_modules/@bsull/augurs/clustering_bg.wasm') }); -initDtwSync({ module: readFileSync('node_modules/@bsull/augurs/dtw_bg.wasm') }); +const CLUSTERING_WASM_PATH = process.env.CLUSTERING_WASM_PATH || 'node_modules/@bsull/augurs/clustering_bg.wasm'; +const DTW_WASM_PATH = process.env.DTW_WASM_PATH || 'node_modules/@bsull/augurs/dtw_bg.wasm'; + +initClusteringSync({ module: readFileSync(CLUSTERING_WASM_PATH) }); +initDtwSync({ module: readFileSync(DTW_WASM_PATH) });js/testpkg/mstl.test.ts (1)
Line range hint
23-27: Consider deprecation strategy for standalone function initialization.While marking the standalone function as deprecated is good, consider:
- Adding a deprecation timeline in the comment
- Creating a migration guide for users
- Eventually removing this test case in a future update
Example comment improvement:
- it('can be instantiated with a (deprecated) standalone function', () => { + it('can be instantiated with a standalone function (deprecated: will be removed in v2.0.0)', () => {js/testpkg/prophet.test.ts (1)
Line range hint
4-14: Consider addressing type compatibility across packages.As mentioned in the PR objectives, there's a known issue where types are expected to work across packages but are treated as distinct after compilation. This could affect the interaction between the Prophet class and the optimizer.
Consider these potential solutions:
- Use WASM components and JCO as suggested in the PR description
- Create a shared types package that both modules depend on
- Document the type compatibility requirements for consumers
Would you like me to elaborate on any of these approaches?
Cargo.toml (1)
44-44: Consider documenting the augurs-core-js dependency's purpose.The addition of
augurs-core-jsas a workspace member suggests it might contain shared types or utilities. Consider documenting its role in addressing the cross-package type handling issue mentioned in the PR description.js/testpkg/dtw.test.ts (1)
42-46: Track re-enablement of parallelization tests.The parallelization test has been commented out due to compilation settings. We should track this to ensure it's re-enabled when parallelization support is added back.
Would you like me to create a GitHub issue to track the re-enablement of parallelization tests? The issue would include:
- Current status and reason for disablement
- Requirements for re-enablement
- Test cases to be restored
js/testpkg/outlier.test.ts (1)
7-7: Consider making the WASM module path configurable.The hardcoded path to
node_modulesmight cause issues in different environments or build setups.Consider using a configuration or environment variable:
-initSync({ module: readFileSync('node_modules/@bsull/augurs/outlier_bg.wasm') }); +const wasmPath = process.env.AUGURS_WASM_PATH || 'node_modules/@bsull/augurs'; +initSync({ module: readFileSync(`${wasmPath}/outlier_bg.wasm`) });js/augurs-changepoint-js/src/lib.rs (2)
10-10: LGTM: Good architectural decision to move VecF64 to core package.This change aligns well with the PR objective of splitting packages by centralizing common types in the core package, which helps prevent type duplication across different WASM modules.
51-53: Consider a more idiomatic approach for JavaScript naming conventions.While
#[allow(non_snake_case)]works, consider these more maintainable alternatives:
- Apply the attribute only to the parameter:
pub fn new( #[allow(non_snake_case)] detectorType: ChangepointDetectorType ) -> Result<ChangepointDetector, JsValue>
- Use serde's rename attribute (preferred):
pub fn new( #[serde(rename = "detectorType")] detector_type: ChangepointDetectorType ) -> Result<ChangepointDetector, JsValue>This would maintain Rust's naming conventions while properly handling JavaScript interop.
js/augurs-dtw-js/src/lib.rs (2)
174-177: Consider documenting the return type change.The change to return
Vec<Float64Array>instead ofDistanceMatrixis a good choice for JavaScript interop. However, it would be helpful to document this change in the method's documentation.Add a note about the return type format, for example:
/// Compute the distance matrix between all pairs of series. /// /// The series do not all have to be the same length. + /// + /// Returns a vector of Float64Array, where each array represents a row in the distance matrix. #[wasm_bindgen(js_name = distanceMatrix)]
181-222: Update package name in documentation example.The documentation example is comprehensive and well-structured, showing both direct and worker-based usage patterns. However, the import statement uses what appears to be an incorrect package name.
Update the import statement in the example:
-import init, { Dbscan, Dtw, initThreadPool} from '@bsull/augurs'; +import init, { Dbscan, Dtw, initThreadPool} from '@grafana/augurs';js/testpkg/tsconfig.json (2)
25-25: Good choice updating to node16 module system!The switch to
"node16"for bothmoduleandmoduleResolutionis well-aligned with the PR's goal of splitting packages into separate WASM modules. This configuration:
- Enables better tree-shaking support in modern bundlers
- Provides native ESM support for efficient WASM module loading
- Positions the codebase well for future WASM component model integration
Also applies to: 27-27
100-100: Consider implications of skipLibCheck with split packages.While
skipLibCheckimproves build performance, it might mask type incompatibilities between your split WASM packages. Consider:
- Temporarily disabling it during development of the package split
- Adding focused type checking for WASM interface boundaries
js/testpkg/prophet.real.test.ts (1)
Line range hint
49-1729: Consider extracting test data to separate fixture files.The test file contains large arrays of test data. Consider moving these to separate fixture files to:
- Improve code readability
- Make the test file more maintainable
- Enable reuse of test data across different test files
Example structure:
+ // fixtures/prophet/training.ts + export const TRAINING_DS = [...]; + export const TRAINING_Y = [...]; + + // fixtures/prophet/prediction.ts + export const PREDICTION_DS = [...]; + export const EXPECTED_YHAT = [...]; + - const TRAINING_DS = [...]; - const TRAINING_Y = [...]; - const PREDICTION_DS = [...]; - const EXPECTED_YHAT = [...]; + import { TRAINING_DS, TRAINING_Y } from '../fixtures/prophet/training'; + import { PREDICTION_DS, EXPECTED_YHAT } from '../fixtures/prophet/prediction';js/augurs-core-js/src/lib.rs (5)
98-99: Visibility change: 'convert' method in 'VecUsize'The
convertmethod inVecUsizehas been made public. Ensure that exposing this method aligns with the intended API design and does not expose unnecessary implementation details.
106-107: Visibility change: 'convert' method in 'VecF64'The
convertmethod inVecF64is now public. Verify that this change is intentional and appropriate for external use.
114-115: Visibility change: 'convert' method in 'VecVecF64'The
convertmethod inVecVecF64has been made public. Consider whether exposing this method is necessary and does not compromise encapsulation.
131-136: Combine multiple 'impl DistanceMatrix' blocks for clarityThere are multiple
impl DistanceMatrixblocks. Combining them into a singleimplblock can improve readability and maintainability.Also applies to: 138-166
140-140: Remove unnecessary '#[allow(non_snake_case)]' attributeThe
#[allow(non_snake_case)]attribute at line 140 appears unnecessary since the function namenewand the parameterdistance_matrixfollow snake_case conventions. Consider removing this attribute.js/augurs-outlier-js/src/lib.rs (3)
8-9: Clarify the import ofOutlierDetectoras_The import statement
use augurs_outlier::OutlierDetector as _;brings theOutlierDetectortrait into scope without directly using it, preventing unused import warnings while enabling trait methods. To improve code readability, consider adding a comment explaining this usage.
139-141: Consider adding unit tests for themaddetectorThe new
madfunction introduces the MAD algorithm. Ensure that unit tests are added to cover this new functionality to verify correctness and prevent regressions.
184-186: Clarify the method documentationThe
detectmethod operates on preprocessed data withinLoadedOutlierDetectorand does not require input parameters. Consider updating the documentation to reflect that it detects outliers using the previously preprocessed data.Suggested change:
- /// Detect outliers in the given time series. + /// Detect outliers using the preprocessed data.js/augurs-prophet-js/src/lib.rs (5)
Line range hint
263-285: Avoid Unnecessary Data Copying inInitialParams::fromCreating new
Float64Arrayinstances and copying data withcopy_fromcan be inefficient for large datasets. Consider usingFloat64Array::fromif the source data can be converted directly orFloat64Array::viewfor zero-copy views when safe.Apply this diff to improve performance:
impl From<&augurs_prophet::optimizer::InitialParams> for InitialParams { fn from(params: &augurs_prophet::optimizer::InitialParams) -> Self { - let delta = Float64Array::new_with_length(params.delta.len() as u32); - delta.copy_from(¶ms.delta); - let beta = Float64Array::new_with_length(params.beta.len() as u32); - beta.copy_from(¶ms.beta); + let delta = unsafe { Float64Array::view(¶ms.delta) }; + let beta = unsafe { Float64Array::view(¶ms.beta) }; Self { k: params.k, m: params.m, delta, beta, sigma_obs: *params.sigma_obs, } } }Note: Ensure that using
unsafecode here does not introduce any safety issues, and that the lifetime ofparams.deltaandparams.betais managed correctly.
Line range hint
710-744: Validate Optional Fields inPredictionDataSimilar to
TrainingData, the optional fields inPredictionDatamust be validated. Check that whencap,floor,seasonality_conditions, orxare provided, they conform to expected formats and lengths. This ensures accurate predictions and prevents potential panics.
765-774: Simplify Option Chaining inmake_forecastFunctionThe nested
Optionchaining in constructingintervalscan be simplified for better readability. Consider refactoring to use pattern matching or theand_thencombinator to make the code clearer.Apply this refactored code:
fn make_forecast(level: Option<f64>, predictions: augurs_prophet::FeaturePrediction) -> Forecast { Forecast { point: predictions.point, - intervals: level.zip(predictions.lower).zip(predictions.upper).map( - |((level, lower), upper)| ForecastIntervals { + intervals: level.and_then(|level| { + predictions.lower.zip(predictions.upper).map(|(lower, upper)| ForecastIntervals { level, lower, upper, - }, - ), + }) + }), } }
71-74: Handle Errors from JavaScript Function Calls AppropriatelyIn the
optimizemethod, if the JavaScript functionself.func.call3fails, the error is converted usingaugurs_prophet::optimizer::Error::string. Ensure that any JavaScript exceptions are properly captured and converted to meaningful Rust errors to aid in debugging.
644-648: Improve Documentation for Data TypesThe type annotation for
dsinTrainingDataincludesTimestampSeconds[] | BigInt64Array. Consider adding examples or further clarification on when each type should be used to assist users in providing the correct input format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
js/testpkg/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (43)
.github/workflows/js.yml(1 hunks)Cargo.toml(2 hunks)crates/augurs-dtw/CHANGELOG.md(1 hunks)crates/augurs-js/.cargo/config.toml(0 hunks)crates/augurs-js/CHANGELOG.md(0 hunks)crates/augurs-js/LICENSE-APACHE(0 hunks)crates/augurs-js/LICENSE-MIT(0 hunks)crates/augurs-js/README.md(0 hunks)crates/augurs-js/prepublish.js(0 hunks)crates/augurs-js/rust-toolchain.toml(0 hunks)js/augurs-changepoint-js/Cargo.toml(1 hunks)js/augurs-changepoint-js/src/lib.rs(2 hunks)js/augurs-clustering-js/Cargo.toml(1 hunks)js/augurs-clustering-js/src/lib.rs(2 hunks)js/augurs-core-js/Cargo.toml(2 hunks)js/augurs-core-js/src/lib.rs(2 hunks)js/augurs-dtw-js/Cargo.toml(1 hunks)js/augurs-dtw-js/src/lib.rs(3 hunks)js/augurs-ets-js/Cargo.toml(1 hunks)js/augurs-ets-js/src/lib.rs(2 hunks)js/augurs-mstl-js/Cargo.toml(1 hunks)js/augurs-mstl-js/src/lib.rs(3 hunks)js/augurs-outlier-js/Cargo.toml(1 hunks)js/augurs-outlier-js/src/lib.rs(5 hunks)js/augurs-prophet-js/Cargo.toml(1 hunks)js/augurs-prophet-js/src/lib.rs(8 hunks)js/augurs-seasons-js/Cargo.toml(1 hunks)js/augurs-seasons-js/src/lib.rs(1 hunks)js/justfile(1 hunks)js/package.json.tmpl(1 hunks)js/testpkg/changepoints.test.ts(1 hunks)js/testpkg/clustering.test.ts(2 hunks)js/testpkg/dtw.test.ts(3 hunks)js/testpkg/ets.test.ts(1 hunks)js/testpkg/logging.test.ts(1 hunks)js/testpkg/mstl.test.ts(2 hunks)js/testpkg/outlier.test.ts(1 hunks)js/testpkg/package.json(1 hunks)js/testpkg/prophet.real.test.ts(2 hunks)js/testpkg/prophet.test.ts(2 hunks)js/testpkg/seasons.test.ts(1 hunks)js/testpkg/tsconfig.json(4 hunks)justfile(2 hunks)
💤 Files with no reviewable changes (7)
- crates/augurs-js/.cargo/config.toml
- crates/augurs-js/CHANGELOG.md
- crates/augurs-js/LICENSE-APACHE
- crates/augurs-js/LICENSE-MIT
- crates/augurs-js/README.md
- crates/augurs-js/prepublish.js
- crates/augurs-js/rust-toolchain.toml
✅ Files skipped from review due to trivial changes (8)
- crates/augurs-dtw/CHANGELOG.md
- js/augurs-changepoint-js/Cargo.toml
- js/augurs-clustering-js/Cargo.toml
- js/augurs-ets-js/Cargo.toml
- js/augurs-mstl-js/Cargo.toml
- js/augurs-prophet-js/Cargo.toml
- js/augurs-seasons-js/Cargo.toml
- js/package.json.tmpl
🔇 Additional comments (56)
js/testpkg/package.json (1)
Line range hint 1-17: Package configuration looks good!
The package configuration is well-structured with:
- Appropriate module type
- Proper test setup with vitest
- TypeScript configuration
- Private flag to prevent accidental publishing
.github/workflows/js.yml (2)
Line range hint 21-22: Verify the build process for individual WASM modules.
Since the PR's goal is to enable packaging individual functionalities as separate WASM modules, please ensure that just build-augurs-js properly handles building and testing each module independently to catch any cross-package type issues.
Let's check the build configuration:
33-33: LGTM! Working directory changes align with the package split.
The path updates from crates/augurs-js/testpkg to js/testpkg correctly reflect the architectural change of splitting JavaScript packages into separate modules.
Let's verify if all JS packages are covered by the tests:
Also applies to: 36-36, 39-39
✅ Verification successful
Test coverage is complete for all JS packages
Based on the test files found in js/testpkg/, there is a dedicated test file for each JS package in the repository:
- changepoints.test.ts -> augurs-changepoint-js
- clustering.test.ts -> augurs-clustering-js
- dtw.test.ts -> augurs-dtw-js
- ets.test.ts -> augurs-ets-js
- logging.test.ts -> augurs-core-js
- mstl.test.ts -> augurs-mstl-js
- outlier.test.ts -> augurs-outlier-js
- prophet.test.ts -> augurs-prophet-js
- seasons.test.ts -> augurs-seasons-js
The working directory change is correct and all packages are properly tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for all JS packages
# Test 1: List all JS packages
echo "Available JS packages:"
fd -t d '^augurs-.*-js$' js/
# Test 2: Check if testpkg imports all packages
echo -e "\nJS package imports in testpkg:"
rg "from ['|\"]@augurs" js/testpkg/
Length of output: 393
Script:
#!/bin/bash
# Let's check the content of testpkg to understand its testing setup
# Check package.json for test dependencies and scripts
echo "=== testpkg/package.json ==="
cat js/testpkg/package.json
# Check for test files
echo -e "\n=== Test files in testpkg ==="
fd -e test.js -e test.ts -e spec.js -e spec.ts . js/testpkg/
# Check imports in all test files
echo -e "\n=== Imports in test files ==="
fd -e test.js -e test.ts -e spec.js -e spec.ts . js/testpkg/ -x cat {}
Length of output: 147054
js/augurs-outlier-js/Cargo.toml (4)
1-11: LGTM: Package metadata is well-structured
The package metadata appropriately uses workspace inheritance and includes a clear description. The publish = false setting correctly prevents accidental publishing.
29-31: Verify browser compatibility with new WASM features
The addition of --enable-bulk-memory and --enable-threads optimizations may affect browser compatibility. Please ensure:
- Target browsers support bulk memory operations
- Threading support is properly handled in the JavaScript integration
Consider documenting browser requirements in the package README.
33-34: LGTM: Consistent lint configuration
Using workspace lints ensures consistent code quality across all packages.
20-27: ```shell
#!/bin/bash
Let's check the actual getrandom configuration in Cargo.toml
cat Cargo.toml | grep -A 5 "getrandom"
Let's also check the wasm32 configuration in justfile
cat components/justfile | grep -A 5 "wasm32"
Check if there are any wasm32 specific configurations in workspace
rg "wasm32" .
</details>
<details>
<summary>js/testpkg/logging.test.ts (2)</summary>
`7-7`: **Verify WASM file packaging and availability.**
The WASM file path has been updated to use `core_bg.wasm`. We should ensure this file is correctly packaged and available at the specified path after npm installation.
```shell
#!/bin/bash
# Description: Verify WASM file packaging configuration
# Test 1: Check if the WASM file is included in package files
echo "Checking package.json for WASM file inclusion..."
fd package.json --exec grep -l "core_bg.wasm"
# Test 2: Check build configuration for WASM file
echo "Checking build configuration..."
fd "webpack|rollup|vite" --exec grep -l "core_bg.wasm"
3-3: Verify package name consistency across the project.
The import path has been updated to use the scoped package @bsull/augurs. Let's verify this package name is used consistently across the project.
✅ Verification successful
Let me try a different search strategy to verify the package naming consistency.
Package naming is consistent and correctly structured
The verification shows that the package naming is consistent across the project:
- The main package is correctly defined as
@bsull/augursin its package.json - The test package correctly lists
@bsull/augursas a dependency - All imports across test files consistently use either
@bsull/augursor its submodules (e.g.,@bsull/augurs/prophet,@bsull/augurs/seasons)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in package naming
# Test 1: Search for package name in package.json files
echo "Checking package.json files for package name..."
fd package.json --exec grep -l "@bsull/augurs"
# Test 2: Search for imports using this package
echo "Checking for imports using this package..."
rg "from ['|\"]@bsull/augurs" --type ts
Length of output: 280
Script:
#!/bin/bash
# Search for package name in package.json files
echo "Checking package.json files..."
fd package.json --exec cat {}
# Search for all import statements in TypeScript files
echo -e "\nChecking TypeScript imports..."
rg "^import.*from" --type ts
Length of output: 5157
js/augurs-core-js/Cargo.toml (4)
2-2: Package name and description align with modularization goals.
The renaming from "augurs-js" to "augurs-core-js" and the updated description better reflect the package's focused scope as part of the split architecture.
Also applies to: 10-10
31-35: Dependencies properly configured for workspace.
The transition to workspace references for key dependencies like serde-wasm-bindgen, tracing, and wasm-bindgen is appropriate for maintaining consistency across split packages.
27-28: Verify if getrandom dependency can be safely removed.
The commented-out getrandom dependency with js feature might be necessary for WASM random number generation in the browser environment.
✅ Verification successful
The getrandom dependency can be safely removed
The codebase analysis shows that:
- No direct usage of
getrandomor random number generation in augurs-core-js - The crate is primarily focused on JavaScript bindings using wasm-bindgen
- The only wasm-bindgen usage is for type definitions, not for random number functionality
- The parent augurs-core crate also doesn't use getrandom
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if getrandom is used in any Rust files
rg -l "getrandom::"
# Check if there are any random number generation uses
ast-grep --pattern 'rand::$_'
Length of output: 304
Script:
#!/bin/bash
# Check for any direct or indirect usage of getrandom in the augurs-core-js crate
rg -l "getrandom" js/augurs-core-js/
# Check for any random number generation in augurs-core-js
rg -l "rand::" js/augurs-core-js/
# Check if wasm-bindgen is used which might need getrandom
cat js/augurs-core-js/Cargo.toml
# Look for any WASM-specific random number generation
rg -l "web_sys.*random" js/augurs-core-js/
Length of output: 1273
Script:
#!/bin/bash
# Check if augurs-core-js uses any random number generation functionality
rg -A 5 "random" js/augurs-core-js/src/
# Check if augurs-core (the dependency) uses getrandom
rg -l "getrandom" crates/augurs-core/
# Check if wasm-bindgen features are used that might require getrandom
rg -A 5 "#\[wasm_bindgen\]" js/augurs-core-js/src/
Length of output: 475
Line range hint 2-43: Consider addressing cross-package type sharing.
Based on the PR objectives, there are issues with types operating across different packages. Consider:
- Using a shared types package
- Implementing type definitions using WASM components
js/justfile (2)
25-31: LGTM! Verify package.json template existence.
The package.json version synchronization logic is robust and well-implemented.
Let's verify the template file exists:
#!/bin/bash
# Verify package.json template exists and has required fields
if [ -f "package.json.tmpl" ]; then
echo "Checking package.json.tmpl structure"
jq -e '.name and .version and .main' package.json.tmpl
fi1-11: Consider enhancing build robustness and dependency handling.
The build target processes multiple packages sequentially. Consider:
- Adding error handling to prevent silent failures
- Documenting the build order if there are inter-package dependencies
- Potentially parallelizing builds for independent packages
Let's check for package dependencies:
✅ Verification successful
Build order is correct and dependencies are properly managed
After analyzing the Cargo.toml files, I can confirm that:
- All packages depend on
augurs-core-js, which is correctly built first in the sequence - Other packages are independent of each other, making their build order non-critical
- The workspace dependencies ensure consistent versioning across packages
The current build order in the justfile is optimal, with the core package being built early. The sequential build approach is appropriate since all packages need to be built successfully for the system to work properly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for cross-dependencies between JS packages
for pkg in changepoint clustering core dtw ets mstl outlier prophet seasons; do
echo "Checking dependencies for augurs-${pkg}-js"
if [ -f "augurs-${pkg}-js/Cargo.toml" ]; then
rg "augurs-.*-js" "augurs-${pkg}-js/Cargo.toml"
fi
done
Length of output: 124
Script:
#!/bin/bash
# Let's try a different approach to check for dependencies
# First, let's check if the packages exist and their structure
fd -t d "augurs-.*-js$"
# Then check Cargo.toml files in js directory and its subdirectories
fd -t f "Cargo.toml$" js/
# Finally, let's examine the content of any Cargo.toml files we find
for toml in $(fd -t f "Cargo.toml$" js/); do
echo "=== Contents of ${toml} ==="
cat "$toml"
echo
done
Length of output: 10480
js/augurs-dtw-js/Cargo.toml (2)
13-18: Consider enabling tests for the JavaScript bindings.
While disabling tests might speed up builds, it's important to ensure the JavaScript bindings work correctly. Consider enabling at least integration tests to verify the WASM interface behaves as expected.
37-39: Verify browser compatibility for WASM features.
The WASM configuration enables advanced features (bulk memory and threads) that might not be supported in all target browsers. Consider:
- Documenting browser compatibility requirements
- Providing fallback options for browsers without thread support
js/testpkg/ets.test.ts (2)
7-7: LGTM! Verify WASM module size reduction.
The WASM module path change reflects the package split strategy. This should result in a smaller WASM module size since it only contains ETS-specific functionality.
#!/bin/bash
# Description: Compare WASM module sizes to verify the split reduces bundle size
# Expected: ets_bg.wasm should be smaller than the original augurs_bg.wasm
# Check size of new ETS WASM module
ls -lh node_modules/@bsull/augurs/ets_bg.wasm 2>/dev/null
# Search for any remaining instances of the original WASM module for comparison
find . -name "augurs_bg.wasm" -exec ls -lh {} \;3-3: LGTM! Verify package availability.
The import change aligns with the PR objective of splitting functionality into separate packages. This modular approach will help reduce bundle sizes for users who only need ETS functionality.
✅ Verification successful
Import path change is correct and package dependency is properly configured
The package @bsull/augurs is correctly listed as a dependency in package.json of the test package, and the import is being used consistently across multiple test files in the codebase. The ETS module is part of the main @bsull/augurs package, following the same pattern as other modules like outlier, prophet, and seasons. The import statement aligns with the established pattern in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the package is properly published and accessible
# Expected: Package should be listed in package.json and available in node_modules
# Check package.json for @bsull/augurs/ets dependency
jq '.dependencies["@bsull/augurs"]' package.json
# Verify the ets module exists in node_modules
ls node_modules/@bsull/augurs/ets* 2>/dev/null
Length of output: 99
Script:
#!/bin/bash
# Let's verify the package structure and dependencies more thoroughly
# Check if package.json exists and examine its content
find . -name "package.json" -type f -exec cat {} \;
# Search for any references to @bsull/augurs in the codebase
rg "@bsull/augurs" --type ts --type js
# Look for any ETS-related imports in the codebase
rg "import.*ets" --type ts --type js
Length of output: 3994
justfile (1)
4-4: Verify the existence of js directory commands.
The commands have been simplified to use js/build, js/test, and js/publish. This aligns well with the package splitting objective and improves maintainability.
Let's verify the existence of these commands:
Also applies to: 7-7, 11-11
✅ Verification successful
The js/ directory structure and commands are properly configured
The verification confirms that:
- The
js/directory exists with its ownjustfilecontaining the required commands - The commands are properly defined:
buildtarget handles multiple packages (changepoint, clustering, core, dtw, ets)testtarget runs npm tests in testpkg directorypublishtarget handles npm publishing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of js directory and its just commands
# Test 1: Check if js directory exists
fd -t d '^js$'
# Test 2: Check for justfile in js directory
fd -t f '^justfile$' -d 2 js/
# Test 3: Look for these command definitions in any justfile
rg -A 5 'build:|test:|publish:' js/
Length of output: 1854
js/augurs-seasons-js/src/lib.rs (1)
7-7: Verify cross-package type compatibility for VecF64
The centralization of VecF64 in augurs_core_js aligns with the PR's objective of modular WASM packaging. However, given the PR objectives mention issues with types operating across different packages, we should verify that VecF64 maintains compatibility when used between packages.
Let's check for potential type compatibility issues:
Consider documenting the cross-package type handling strategy and potentially adding runtime type checks to catch any mismatches early. This is especially important for WASM interop where type mismatches might not be caught at compile time.
✅ Verification successful
Based on the output, I can now generate a final response as we have comprehensive information about VecF64 usage across packages.
VecF64 type compatibility is properly maintained across packages
The centralization of VecF64 in augurs_core_js is working as intended, with proper type conversion handling:
VecF64is defined as a custom type inaugurs_core_jswith WASM bindings- All packages consistently use
convert()method to safely transform between JS/WASM types - Type conversion is properly handled through
serde_wasm_bindgenfor safe serialization - The implementation is uniform across all dependent packages (seasons, mstl, ets, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze VecF64 usage across packages to identify potential compatibility issues
# Check how VecF64 is defined in augurs_core_js
echo "Checking VecF64 definition in augurs_core_js:"
rg -p "struct VecF64" --type rust
# Check all locations where VecF64 is used across packages
echo -e "\nChecking VecF64 usage patterns:"
rg -p "VecF64" --type rust -A 3
# Look for any type conversion or serialization code related to VecF64
echo -e "\nChecking for type conversions:"
rg -p "impl.*for.*VecF64" --type rust -A 5
Length of output: 4068
js/testpkg/clustering.test.ts (3)
3-4: LGTM! Clean separation of clustering and DTW imports.
The split imports align well with the PR objective of enabling individual WASM module imports.
19-24: LGTM! Simplified input handling with comprehensive test coverage.
The changes improve API usability by:
- Accepting raw arrays directly instead of requiring DistanceMatrix objects
- Supporting both regular arrays and typed arrays
- Maintaining consistent behavior with previous implementation
Also applies to: 30-35
Line range hint 40-50: Verify cross-package type compatibility.
The test case combining DTW and clustering functionality is crucial for verifying cross-package type compatibility, which was mentioned as a concern in the PR objectives.
Let's check for other cross-package type usage:
✅ Verification successful
Cross-package type compatibility verified successfully
The test case demonstrates proper type compatibility between DTW and clustering packages:
- DTW's distance matrix output is correctly consumed by DbscanClusterer
- Both packages handle typed arrays (Float64Array) and regular arrays consistently
- No type errors or conversions needed between the packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports that might indicate cross-package type usage
rg -l "@bsull/augurs/(clustering|dtw)" --type ts
# Search for potential type definitions that might be shared
ast-grep --pattern 'type $name = $$$' --type ts
ast-grep --pattern 'interface $name { $$$ }' --type ts
Length of output: 570
Script:
#!/bin/bash
# Let's fix the ast-grep commands and search for type definitions
ast-grep --pattern 'type $name = $$$'
ast-grep --pattern 'interface $name { $$$ }'
# Check the content of test files to understand the cross-package usage
cat js/testpkg/dtw.test.ts
cat js/testpkg/clustering.test.ts
# Look for any type imports
rg "import.*type" --type ts
Length of output: 569
Script:
#!/bin/bash
# Something went wrong with the previous commands. Let's try a different approach
# First, let's check the actual content of the test files
find . -name "*.test.ts" -exec cat {} \;
# Then look for type-related imports specifically in these test files
find . -name "*.test.ts" -exec rg "import.*{.*}.*from.*@bsull" {} \;
# And check for any type definitions in the source files
find . -name "*.ts" ! -name "*.test.ts" -exec rg "type|interface" {} \;
Length of output: 147402
Script:
#!/bin/bash
# Let's try a simpler approach to check imports and type usage
# Check imports between DTW and clustering
rg "import.*from.*@bsull/augurs/(clustering|dtw)" --type ts
# Look for type definitions in the test files
rg "type|interface" js/testpkg/clustering.test.ts js/testpkg/dtw.test.ts
Length of output: 742
js/testpkg/mstl.test.ts (3)
Line range hint 11-58: Test suite structure is comprehensive and well-maintained.
The test suite effectively covers:
- Multiple initialization methods
- Different input data types (regular arrays, typed arrays)
- In-sample predictions and confidence intervals
- Return type validations
7-7: Verify WASM file generation and placement.
The WASM module path has been updated to use the specific MSTL module. This supports the goal of allowing users to import only the functionality they need.
Let's verify the WASM file setup:
#!/bin/bash
# Description: Verify WASM file generation and placement
# Test 1: Check if the WASM file exists
echo "Checking for WASM file..."
ls -la node_modules/@bsull/augurs/mstl_bg.wasm
# Test 2: Check for any other WASM files that might need similar updates
echo "Checking for other WASM files..."
find . -name "*_bg.wasm"3-3: Verify package publication and accessibility.
The import path has been updated to use the published package @bsull/augurs/mstl. This aligns with the PR objective of splitting functionality into separate modules.
Let's verify the package setup:
✅ Verification successful
Package import path is correct but requires local dependency setup
The import path @bsull/augurs/mstl is correct according to the package template's exports configuration. However, the test package is currently using a local dependency ("@bsull/augurs": "../augurs") instead of a published version, which explains why the WASM files weren't found in node_modules. This is expected in a development environment where packages are being split into separate crates.
- The import statement aligns with the final published structure as defined in
js/package.json.tmpl - The test package correctly references the local development version for testing
- WASM files will be included in the final published package as specified in the
filesfield of the template
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package.json configuration and package existence
# Test 1: Check if the package is listed in package.json
echo "Checking package.json for @bsull/augurs..."
cat package.json | grep -A 5 "dependencies"
# Test 2: Check if the package directory exists in node_modules
echo "Checking if package is installed..."
ls -la node_modules/@bsull/augurs/
Length of output: 393
Script:
#!/bin/bash
# Let's try a different approach to verify the package setup
# Check if package.json exists and its content
echo "=== Checking package.json content ==="
if [ -f "package.json" ]; then
cat package.json
else
echo "package.json not found"
fi
# Search for any references to @bsull/augurs in the codebase
echo -e "\n=== Searching for package references ==="
rg "@bsull/augurs" --type ts --type js
# Check for any WASM-related files
echo -e "\n=== Looking for WASM files ==="
fd -e wasm
# Check for any package configuration files
echo -e "\n=== Looking for package configuration files ==="
fd "package.json|.npmrc"
Length of output: 3044
Script:
#!/bin/bash
# Let's check the test package configuration and WASM file locations
# Check test package configuration
echo "=== Checking js/testpkg/package.json ==="
cat js/testpkg/package.json
# Check if the WASM files are in a different location
echo -e "\n=== Checking js/testpkg directory for WASM files ==="
ls -la js/testpkg/
# Check the package.json template
echo -e "\n=== Checking package.json template ==="
cat js/package.json.tmpl
# Look for any build or dist directories that might contain the WASM files
echo -e "\n=== Looking for build/dist directories ==="
fd -t d "build|dist|pkg"
Length of output: 2857
js/testpkg/prophet.test.ts (2)
14-14: Verify WASM module path and availability.
The WASM module path has been updated to load the prophet-specific module. This change supports the package splitting strategy, but we need to ensure the module is available at the specified path after installation.
#!/bin/bash
# Description: Verify WASM module availability and packaging
# Test 1: Check if the WASM file is included in the package
rg -l "prophet_bg.wasm" package.json
# Test 2: Check build configuration to ensure WASM file is properly packaged
fd -g "*.toml" | xargs rg "prophet_bg.wasm"4-5: Verify package dependencies are correctly specified.
The import paths have been updated to use the new package structure. While this aligns with the PR's objective of splitting packages, we should verify that these dependencies are properly declared.
✅ Verification successful
Based on the output, I can now generate the final response since I have sufficient information. The package.json in the testpkg directory shows that both required dependencies are correctly specified:
"devDependencies": {
"@bsull/augurs": "../augurs",
"@bsull/augurs-prophet-wasmstan": "^0.2.0",
...
}Dependencies are correctly specified ✓
The package dependencies align with the import statements:
@bsull/augursis specified as a local path dependency (../augurs)@bsull/augurs-prophet-wasmstanis specified with a proper version (^0.2.0)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package dependencies are correctly specified in package.json
# Test: Check if the required packages are listed in package.json
rg -l '"@bsull/augurs":|"@bsull/augurs-prophet-wasmstan":' package.json
Length of output: 133
Script:
#!/bin/bash
# Let's check the repository structure and package configuration files
fd -g 'package.json' -g 'Cargo.toml'
# Then check the content of any package.json files for dependencies
fd -g 'package.json' -x cat {}
# Also check Cargo.toml files since this seems to be a Rust/WASM project
fd -g 'Cargo.toml' -x cat {}
# Let's also verify if these modules actually exist in the codebase
fd -g '*prophet*.{ts,js,rs}'
Length of output: 25561
js/augurs-ets-js/src/lib.rs (2)
25-25: LGTM! Consider applying consistently across the codebase.
The #[allow(non_snake_case)] attribute is appropriate for JavaScript bindings as it allows using idiomatic camelCase naming.
Let's check for consistency across other JS binding files:
#!/bin/bash
# Description: Check if other JS binding constructors use the same attribute
# Find all Rust files in js directories that might need the same attribute
fd -e rs . 'js/' --exec rg -l 'pub fn new.*[A-Z]'
# Look for existing usage of the attribute
fd -e rs . 'js/' --exec rg -l '#\[allow\(non_snake_case\)\]'6-6: LGTM! Verify the new import path.
The change to import from augurs_core_js aligns with the PR's objective of splitting packages.
Let's verify the package accessibility:
✅ Verification successful
Import path change is correctly implemented
The verification confirms that:
- The
augurs-core-jscrate exists and is properly set up - Both
ForecastandVecF64types are defined and exported injs/augurs-core-js/src/lib.rs - The import change aligns with the project's modularization goals, moving JS-specific types to a dedicated crate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that augurs_core_js is properly configured and accessible
# Test 1: Check if the package is defined in workspace
rg "augurs[-_]core[-_]js" -g "Cargo.toml"
# Test 2: Verify the exported types exist
ast-grep --pattern 'pub struct Forecast'
ast-grep --pattern 'pub struct VecF64'
Length of output: 547
Script:
#!/bin/bash
# Let's try a different approach to verify the imports
# Check if augurs-core-js crate exists and its Cargo.toml
fd -g "augurs-core-js/Cargo.toml"
# Check the exports in augurs-core-js
fd -e rs . js/augurs-core-js/ --exec rg -l "pub (struct|type) (Forecast|VecF64)"
# Look for the actual struct definitions
rg "pub (struct|type) (Forecast|VecF64)" -g "*.rs" -A 2
Length of output: 1450
js/testpkg/changepoints.test.ts (2)
Line range hint 9-58: Test suite structure and coverage look good!
The test suite provides comprehensive coverage:
- Tests both initialization approaches (constructor and static methods)
- Validates typed array support
- Covers both normalGamma and defaultArgpcp algorithms
- Includes appropriate assertions for changepoint detection
3-3: Verify package publication and accessibility.
The import path has been updated to use the scoped package @bsull/augurs/changepoint. This aligns with the modularization goal, but we should ensure the package is properly published and accessible.
✅ Verification successful
Import path change is consistent with the codebase structure
The import path @bsull/augurs/changepoint follows the same pattern used consistently across all test files in the js/testpkg directory. All related test files use similar scoped imports from @bsull/augurs/* packages. The presence of package.json.tmpl and the consistent import pattern across test files suggests this is part of a well-structured modularization effort.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package.json dependencies and package availability
# Test 1: Check if the package is listed in package.json
echo "Checking package.json for @bsull/augurs dependency..."
jq '.dependencies["@bsull/augurs"]' package.json
# Test 2: Check if other test files are using consistent import paths
echo "Checking for consistent import paths across test files..."
rg -l "from '@bsull/augurs/" js/testpkg/
Length of output: 654
Script:
#!/bin/bash
# Let's check the package.json files in the repository and workspace configuration
echo "Searching for package.json files..."
fd package.json
# Check the content of package.json files if found
echo -e "\nChecking content of js/package.json (if exists)..."
cat js/package.json 2>/dev/null || echo "File not found"
echo -e "\nChecking for workspace configuration..."
fd "workspace|lerna|rush" -e json -e yaml -e yml
# Let's also check the import statements in other test files to understand the pattern
echo -e "\nAnalyzing import patterns in test files..."
rg "from ['\"](\.\.?/|@bsull/augurs/)" js/testpkg/ -A 1
Length of output: 2027
Cargo.toml (2)
50-65: Verify wasm-bindgen version pinning and consider type sharing strategy.
The exact version pinning of wasm-bindgen (=0.2.93) suggests version sensitivity. Additionally, the combination of serde-wasm-bindgen, js-sys, and tsify-next indicates proper WASM type handling setup.
To address the cross-package type issue mentioned in the PR description, consider:
- Using
augurs-core-jsas a central type definition package - Leveraging
tsify-nextfor consistent type generation
Let's verify the dependency setup:
#!/bin/bash
# Check for consistent WASM dependency versions across JS packages
echo "Checking WASM dependency versions across packages..."
fd Cargo.toml js/ -x grep -H "wasm-bindgen\|js-sys\|serde-wasm-bindgen"
echo "Checking for type definitions..."
fd '\.rs$' js/augurs-core-js -x grep -l "^#\[wasm_bindgen\]"5-10: LGTM! Workspace configuration properly handles JS packages.
The workspace configuration correctly includes JS modules while excluding non-crate JS packages. This aligns with the PR objective of splitting JS packages into separate modules.
Let's verify the workspace structure:
✅ Verification successful
Workspace configuration is correctly structured and excludes are appropriate
The verification confirms that:
- The excluded directory
js/testpkgexists and contains only JS/TS test files, not Rust crates - The directory
js/augursdoesn't exist, but instead individual augurs-*-js crates are properly organized - All augurs-*-js directories contain Cargo.toml files, indicating they are proper Rust crates
- The workspace configuration correctly includes these crates while excluding the pure JS/TS test package
The exclude patterns in Cargo.toml are working as intended, preventing Cargo from treating the JS test package as a Rust crate while properly including all the WASM module crates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify workspace structure and package organization
# Check for presence of JS packages
echo "Checking JS package structure..."
fd -t d '^augurs' js/
# Verify no Cargo.toml exists in excluded directories
echo "Verifying excluded directories..."
[ ! -f "js/augurs/Cargo.toml" ] && [ ! -f "js/testpkg/Cargo.toml" ] && echo "OK: No Cargo.toml in excluded dirs"
Length of output: 2138
Script:
#!/bin/bash
# Let's verify the workspace structure more thoroughly
# Check all directories under js/
echo "Listing all directories under js/:"
ls -la js/
# Check which directories under js/ have Cargo.toml files
echo -e "\nFinding Cargo.toml files under js/:"
fd Cargo.toml js/
# Specifically check for augurs and testpkg
echo -e "\nChecking specifically for augurs and testpkg directories:"
ls -la js/augurs js/testpkg 2>/dev/null || echo "Directories not found as expected"
Length of output: 2697
js/augurs-mstl-js/src/lib.rs (3)
Line range hint 79-83: LGTM! Well-structured implementation of in-sample forecasting.
The new predictInSample method:
- Follows consistent error handling patterns
- Properly converts between Rust and JavaScript types
- Uses camelCase naming as required for JavaScript interop
11-11: Verify type compatibility across packages.
The PR objectives mention potential issues with types operating across different packages. Since Forecast, VecF64, and VecUsize are now imported from augurs_core_js, we should verify that these types maintain compatibility when used across different WASM modules.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Type compatibility is maintained across packages
The types Forecast, VecF64, and VecUsize are consistently used across the JavaScript bindings and core packages:
Forecastis properly defined inaugurs-coreand reexported throughaugurs_core_jsfor all JS packagesVecF64andVecUsizeare consistently defined as WASM types inaugurs-core-jsand imported correctly by other JS packages- Type conversions are handled safely through proper
Into/Fromimplementations andconvert()methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these types are used consistently across other JS packages
# Look for any potential type mismatches or inconsistencies
# Search for type usage patterns
echo "Checking Forecast type usage:"
ast-grep --pattern 'Forecast'
echo "Checking VecF64 type usage:"
ast-grep --pattern 'VecF64'
echo "Checking VecUsize type usage:"
ast-grep --pattern 'VecUsize'
# Look for potential type conversion or serialization code
echo "Checking for type conversion patterns:"
rg -A 3 'convert\(\)|Into::into|From::from'
Length of output: 10813
Line range hint 116-121: Ensure deprecation notice is visible in JavaScript.
The Rust deprecation is well-documented, but we should verify that JavaScript users will see the deprecation warning when using the standalone ets function.
Consider adding a console warning in the deprecated function to ensure JavaScript users are properly notified:
pub fn ets(periods: VecUsize, options: Option<ETSOptions>) -> Result<MSTL, JsError> {
+ web_sys::console::warn_1(&JsValue::from_str("Warning: ets() is deprecated. Use MSTL.ets() instead."));
MSTL::ets(periods, options)
}js/testpkg/dtw.test.ts (2)
116-120: Verify distanceMatrix return type changes.
The test assertions have been updated to handle array types directly instead of using shape() and toArray() methods. This suggests a change in the return type of the distanceMatrix function.
The new assertions provide better type checking, but let's verify this change is consistent across the codebase:
#!/bin/bash
# Description: Check for consistent distanceMatrix return type handling
# Look for other distanceMatrix usages
echo "Checking distanceMatrix usage patterns..."
rg -A 3 "distanceMatrix\("
# Look for any remaining shape() or toArray() calls that might need updating
echo "Checking for potentially outdated method calls..."
rg -l "\.shape\(\)|\.toArray\(\)"Also applies to: 127-133
3-3: Verify package paths after split.
The import path and WASM module initialization have been updated to reflect the new package structure. Let's verify these paths are correct:
Also applies to: 7-7
js/testpkg/outlier.test.ts (2)
Line range hint 39-39: Clarify status of commented clusterBand expectations.
There are multiple commented-out expectations for clusterBand throughout the test file. This could be related to the type issues mentioned in the PR description. Please clarify if these are:
- Temporarily disabled due to the package split
- No longer valid expectations
- Planned for future implementation
If these are related to cross-package type issues, we should track this to ensure it's resolved before the modularization is complete.
Let's check if this is a broader pattern:
#!/bin/bash
# Search for other commented-out clusterBand related code
rg "clusterBand" --type ts -C 2Also applies to: 40-40, 83-83, 84-84, 122-122, 123-123, 147-147, 148-148
3-3: LGTM! Verify import path consistency across packages.
The change from relative to scoped package import aligns with the modularization goal.
Let's verify the consistency of import paths across the codebase:
✅ Verification successful
Import paths have been consistently updated across all test files
The verification shows that:
- No relative imports from '../pkg' remain in the codebase
- All test files consistently use the new scoped package pattern '@bsull/augurs/*'
- Each algorithm (outlier, prophet, seasons, etc.) correctly imports from its dedicated package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining relative imports from '../pkg'
rg "from '\.\./pkg'" --type ts
# Verify the new import pattern is used consistently
rg "from '@bsull/augurs/" --type ts
Length of output: 3962
js/augurs-changepoint-js/src/lib.rs (1)
Line range hint 92-92: Verify type compatibility across WASM boundaries.
Given the PR objective's mention of type-related issues across packages, we should verify that the type conversions (especially for VecF64 and complex types like NormalGammaParameters) work correctly across WASM module boundaries.
✅ Verification successful
Let me analyze the results and generate a final response.
Type compatibility across WASM boundaries appears to be well-handled
The codebase shows a comprehensive and consistent type conversion system:
- No conflicting
VecF64implementations were found - Type conversions for WASM boundaries are properly implemented with explicit
From/TryFromtraits - The
wasm_bindgenannotations are consistently used across JS-facing packages
The type conversion implementations in js/augurs-changepoint-js/src/lib.rs align with the established patterns seen in other packages like augurs-prophet-js and augurs-core-js, suggesting proper type handling across WASM boundaries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential type compatibility issues across packages
# Test 1: Look for other VecF64 implementations that might conflict
echo "Checking for potential VecF64 implementations..."
rg -l "struct VecF64"
# Test 2: Check for type conversion implementations that might need updates
echo "Checking for type conversions that might need updates..."
rg "impl (TryFrom|From|Into)<.*> for"
# Test 3: Look for wasm_bindgen type annotations that might need alignment
echo "Checking for wasm_bindgen type annotations..."
rg "#\[wasm_bindgen\].*struct|enum"
Length of output: 12933
js/augurs-dtw-js/src/lib.rs (2)
108-108: LGTM: Appropriate use of attribute for JavaScript interop.
The #[allow(non_snake_case)] attribute is correctly used to maintain JavaScript naming conventions in the WASM interface.
10-10: Verify type compatibility across package boundaries.
The move of DistanceMatrix, VecF64, and VecVecF64 to augurs_core_js aligns with the modularization goal. However, per PR objectives, there's a known issue where types across different packages are treated as distinct after WASM compilation.
✅ Verification successful
Based on the search results, I can now make a definitive assessment. The types DistanceMatrix, VecF64, and VecVecF64 are properly handled across package boundaries:
augurs_core_jsprovides proper conversion implementations between JS and Rust types:
impl From<augurs_core::DistanceMatrix> for DistanceMatrix {...}
impl From<DistanceMatrix> for augurs_core::DistanceMatrix {...}- The types are consistently used across packages with proper imports from
augurs_core_js:
use augurs_core_js::{DistanceMatrix, VecF64, VecVecF64};- Proper conversion methods are implemented:
pub fn convert(self) -> Result<Vec<f64>, JsError>
pub fn convert(self) -> Result<Vec<Vec<f64>>, JsError>No type compatibility issues found across package boundaries.
The types are properly handled with explicit conversions and implementations to maintain type safety between JavaScript and Rust. The PR's modularization approach correctly maintains type compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential type compatibility issues across packages
# Test 1: Look for other packages using these types
echo "Checking usage of core types across packages..."
rg "DistanceMatrix|VecF64|VecVecF64" --type rust
# Test 2: Look for potential type conversion or casting code that might indicate compatibility workarounds
echo "Checking for type conversion patterns..."
rg "as_|into|from|convert" --type rust
Length of output: 63128
js/testpkg/tsconfig.json (1)
74-74: Verify type compatibility across split packages.
While the strict type checking configuration is good, the PR description mentions issues with types not working across different packages. Consider verifying:
- Type definitions accessibility across package boundaries
- Potential need for shared type definitions
- WASM type conversion consistency
Also applies to: 76-76, 78-78
✅ Verification successful
Type configuration is appropriate for cross-package compatibility
The current TypeScript configuration is well-suited for cross-package type compatibility:
strict: trueensures robust type checkingesModuleInteropandmodule: node16provide proper module resolution- Type definitions are properly declared in package.json (
augurs.d.tsandprophet-wasmstan.d.ts) - Module resolution is correctly set to
node16
No additional type system configurations are needed as the current setup already follows TypeScript best practices for package interoperability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for type definition files and their exports
echo "Checking for type definition files..."
fd -e d.ts -x cat {} \; | grep -B 2 -A 2 "export"
# Look for potential type sharing patterns
echo "Checking for shared type patterns..."
rg -g "*.ts" -g "*.tsx" "type\s+\w+\s*=" -A 2
Length of output: 268
Script:
#!/bin/bash
# Let's try a different approach to check type configurations and sharing
# Check all tsconfig files to understand type configuration across packages
echo "Checking all tsconfig files..."
fd tsconfig.json -x cat {}
# Look for type imports/exports patterns
echo "Checking type import/export patterns..."
rg -g "*.ts" -g "*.tsx" "import.*type|export.*type" -B 1 -A 1
# Check for potential type references in package.json files
echo "Checking package.json type references..."
fd package.json -x cat {} | jq -r 'select(.types != null or .typings != null) | [input_filename, .types, .typings] | @json'
Length of output: 12744
js/testpkg/prophet.real.test.ts (2)
Line range hint 17-47: LGTM! Test implementation remains robust.
The test implementation is well-structured with:
- Proper instantiation tests
- Custom matcher for floating-point comparisons with appropriate error margins
- Comprehensive test cases that remain valid with the new package structure
4-5: Verify package dependencies and WebAssembly module path.
The changes to import paths and WebAssembly module initialization reflect the new package structure. However, we should verify that these dependencies are correctly configured in the project's package.json.
Also applies to: 14-14
✅ Verification successful
Let me try a different approach to verify the package structure since we're in a test environment.
Let me check the package configuration in the test package to verify the dependencies are properly set up.
Import paths and dependencies are correctly configured
The package configuration aligns with the new modular structure:
@bsull/augursis correctly linked as a local dependency (../augurs)@bsull/augurs-prophet-wasmstanis properly specified with version^0.2.0- The imports in the test file match the package structure and dependencies
- The WebAssembly module path is consistent with other test files in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package dependencies and module paths
# Test 1: Check if the packages are listed in package.json
echo "Checking package.json for dependencies..."
cat package.json | jq '.dependencies["@bsull/augurs"] , .dependencies["@bsull/augurs-prophet-wasmstan"]'
# Test 2: Verify the WebAssembly module exists at the specified path
echo "Verifying WebAssembly module path..."
test -f "node_modules/@bsull/augurs/prophet_bg.wasm" && echo "WebAssembly module found" || echo "WebAssembly module not found"
Length of output: 447
Script:
#!/bin/bash
# Let's check for workspace-level package configuration
echo "Checking for workspace package.json..."
fd -t f "package.json"
# Check if these are workspace packages
echo -e "\nChecking for packages directory structure..."
fd -t d "augurs|augurs-prophet"
# Look for import statements using these packages
echo -e "\nChecking usage of these packages across codebase..."
rg "@bsull/augurs|@bsull/augurs-prophet" --type ts
Length of output: 3407
Script:
#!/bin/bash
# Check the package.json in the testpkg directory
echo "Checking js/testpkg/package.json contents..."
cat js/testpkg/package.json
# Check if the prophet-wasmstan package exists and its configuration
echo -e "\nChecking prophet-wasmstan package..."
cat components/js/prophet-wasmstan/package.json
Length of output: 1543
js/augurs-clustering-js/src/lib.rs (2)
7-7: Updated import statement is correct
The import now correctly references DistanceMatrix and VecVecF64 from augurs_core_js.
46-48: Verify that all calls to fit are updated to the new signature
Ensure that all usages of DbscanClusterer.fit throughout the codebase are updated to match the new method signature accepting VecVecF64 and handling the Result return type.
You can run the following script to find all calls to fit:
js/augurs-core-js/src/lib.rs (1)
1-1: Updated module documentation provides clearer description
The updated module-level documentation comment clearly states the purpose of the module.
js/augurs-outlier-js/src/lib.rs (2)
1-1: Module documentation added
The module-level documentation comment provides a clear description of the purpose of the module.
101-103: Enum variants documented correctly
Adding documentation comments for the Dbscan and Mad variants enhances code clarity and usability.
js/augurs-prophet-js/src/lib.rs (3)
71-71: Ensure Proper Conversion of Initial Parameters
The conversion of init to InitialParams using init.into() assumes that all fields are correctly mapped. Verify that the From implementation for InitialParams covers all necessary fields and handles any potential data inconsistencies.
1-10: Verify Import Sources for Forecast Types
The imports for Forecast and ForecastIntervals have been changed to augurs_core_js. Ensure that these types are fully compatible with the expectations in this module and that their implementations align with the required functionalities.
728-735: Validate Seasonality Conditions in PredictionData
When seasonality_conditions are provided in PredictionData, ensure that they match the seasonalities defined in the model and that the length of each condition array corresponds to the number of timestamps ds.
| @@ -3,7 +3,7 @@ | |||
| "type": "module", | |||
| "private": true, | |||
| "devDependencies": { | |||
| "@bsull/augurs": "../pkg", | |||
| "@bsull/augurs": "../augurs", | |||
There was a problem hiding this comment.
💡 Codebase verification
The dependency path "../augurs" appears incorrect
The directory structure shows that there is no augurs directory in the js/ folder. Instead, there are several split packages like augurs-core-js, augurs-prophet-js, etc. This indicates that the dependency path in js/testpkg/package.json needs to be updated to point to the correct package, likely augurs-core-js or another specific augurs package.
- Update the dependency path in
js/testpkg/package.jsonto point to the correct augurs package based on the actual requirements of the test package.
🔗 Analysis chain
Verify the updated dependency path.
The dependency path has been updated from "../pkg" to "../augurs" as part of the package restructuring. Let's verify that this new path is correct and the package exists.
Let me gather more information about the package structure to better understand the context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the augurs package and its package.json
# Expected: The package.json should exist in the new location
# Check if the augurs directory exists
if [ -d "js/augurs" ]; then
echo "✓ Directory js/augurs exists"
# Check if package.json exists in the new location
if [ -f "js/augurs/package.json" ]; then
echo "✓ package.json exists in js/augurs"
# Display the name from package.json to verify it's the correct package
echo "Package name from js/augurs/package.json:"
cat "js/augurs/package.json" | grep '"name":'
else
echo "✗ package.json not found in js/augurs"
fi
else
echo "✗ Directory js/augurs not found"
fi
Length of output: 95
Script:
#!/bin/bash
# Let's check the actual directory structure and look for any package.json files
echo "Directory structure in js/:"
ls -la js/
echo -e "\nSearching for package.json files:"
fd package.json
echo -e "\nChecking the content of js/testpkg/package.json:"
cat js/testpkg/package.json
Length of output: 1687
| @@ -1,10 +1,10 @@ | |||
| import { readFileSync } from "node:fs"; | |||
|
|
|||
| import { seasonalities, initSync } from '../pkg'; | |||
| import { seasonalities, initSync } from '@bsull/augurs/seasons'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Package is published but dependency configuration needs attention
The @bsull/augurs package is published on npm with multiple versions available (latest being 0.5.4), but it's not properly configured in the project's dependencies. The test file assumes the package and its WASM module will be available in node_modules, but:
- No package.json in the project root has the dependency configured
- The package.json files found (
js/testpkg/package.jsonand others) don't list the dependency - The WASM module path
node_modules/@bsull/augurs/seasons_bg.wasmis referenced but won't be available without proper dependency setup
Please add @bsull/augurs to the appropriate package.json and ensure the dependency is installed before running the tests.
🔗 Analysis chain
Verify package availability and versioning.
The change from relative to absolute imports assumes the @bsull/augurs/seasons package is published and available.
Let's verify the package setup:
Let me search for package configuration files in other locations since package.json wasn't found in the root.
Let me check the actual import changes and surrounding context in the test file to better understand the dependency setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package.json dependencies and package availability
# Test 1: Check if package.json includes the dependency
echo "Checking package.json for @bsull/augurs..."
cat package.json | jq '.dependencies["@bsull/augurs"]'
# Test 2: Verify the package is published
echo "Checking npm registry..."
npm view @bsull/augurs versions --json
Length of output: 792
Script:
#!/bin/bash
# Search for package.json files and check their content
fd package.json --type f --exec sh -c 'echo "=== {} ==="; cat {} | jq ".dependencies"'
# Also check for yarn.lock or pnpm-lock.yaml to verify dependency resolution
fd "yarn.lock|pnpm-lock.yaml" --type f --exec echo "Found: {}"
Length of output: 333
Script:
#!/bin/bash
# Check the test file content and git history
echo "=== Current content of seasons.test.ts ==="
cat js/testpkg/seasons.test.ts
echo -e "\n=== Git history of the file ==="
git log -p js/testpkg/seasons.test.ts
Length of output: 2560
js/justfile
Outdated
| publish: | ||
| cd augurs && \ | ||
| node prepublish && \ | ||
| npm publish --access public |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance publish safety measures.
The publish process could benefit from additional safety measures:
- Version confirmation before publishing
- Git tag creation for releases
- Documentation of the prepublish script's purpose
Consider this safer publish process:
publish:
+ #!/usr/bin/env bash
+ set -euo pipefail
+ VERSION=$(cargo metadata --format-version 1 | jq -r '.packages[] | select (.name == "augurs") | .version')
+ echo "Publishing version $VERSION - Are you sure? (y/N)"
+ read -r response
+ if [ "$response" != "y" ]; then
+ echo "Aborting publish"
+ exit 1
+ fi
cd augurs && \
node prepublish && \
- npm publish --access public
+ npm publish --access public && \
+ git tag -a "v$VERSION" -m "Release v$VERSION" && \
+ git push origin "v$VERSION"Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| initSync({ module: readFileSync('node_modules/@bsull/augurs/augurs_bg.wasm') }); | ||
| initSync({ module: readFileSync('node_modules/@bsull/augurs/changepoint_bg.wasm') }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving WASM module loading.
The current implementation has several potential improvements:
- The hard-coded path assumes a specific node_modules structure
- Missing error handling for cases where the WASM file is not found
- Synchronous file loading could block the main thread
Consider this more robust implementation:
-initSync({ module: readFileSync('node_modules/@bsull/augurs/changepoint_bg.wasm') });
+try {
+ // Consider using import.meta.resolve or require.resolve for more reliable path resolution
+ const wasmPath = require.resolve('@bsull/augurs/changepoint_bg.wasm');
+ initSync({ module: readFileSync(wasmPath) });
+} catch (error) {
+ console.error('Failed to load WASM module:', error);
+ throw new Error('Failed to initialize changepoint detection module');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initSync({ module: readFileSync('node_modules/@bsull/augurs/changepoint_bg.wasm') }); | |
| try { | |
| // Consider using import.meta.resolve or require.resolve for more reliable path resolution | |
| const wasmPath = require.resolve('@bsull/augurs/changepoint_bg.wasm'); | |
| initSync({ module: readFileSync(wasmPath) }); | |
| } catch (error) { | |
| console.error('Failed to load WASM module:', error); | |
| throw new Error('Failed to initialize changepoint detection module'); | |
| } |
| impl From<DistanceMatrix> for augurs_core::DistanceMatrix { | ||
| fn from(matrix: DistanceMatrix) -> Self { | ||
| Self::try_from_square(matrix.inner.into_inner()).unwrap() | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid using 'unwrap()' to prevent potential panics
In the implementation of From<DistanceMatrix> for augurs_core::DistanceMatrix, using unwrap() may lead to a panic if try_from_square fails. To prevent potential runtime panics, handle the error explicitly or implement TryFrom instead.
Apply this diff to handle potential errors:
-impl From<DistanceMatrix> for augurs_core::DistanceMatrix {
- fn from(matrix: DistanceMatrix) -> Self {
- Self::try_from_square(matrix.inner.into_inner()).unwrap()
- }
-}
+impl TryFrom<DistanceMatrix> for augurs_core::DistanceMatrix {
+ type Error = JsError;
+ fn try_from(matrix: DistanceMatrix) -> Result<Self, Self::Error> {
+ augurs_core::DistanceMatrix::try_from_square(matrix.inner.into_inner())
+ .map_err(|e| JsError::new(&e.to_string()))
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl From<DistanceMatrix> for augurs_core::DistanceMatrix { | |
| fn from(matrix: DistanceMatrix) -> Self { | |
| Self::try_from_square(matrix.inner.into_inner()).unwrap() | |
| } | |
| } | |
| impl TryFrom<DistanceMatrix> for augurs_core::DistanceMatrix { | |
| type Error = JsError; | |
| fn try_from(matrix: DistanceMatrix) -> Result<Self, Self::Error> { | |
| augurs_core::DistanceMatrix::try_from_square(matrix.inner.into_inner()) | |
| .map_err(|e| JsError::new(&e.to_string())) | |
| } | |
| } |
| @@ -115,7 +117,7 @@ impl OutlierDetector { | |||
| /// Create a new outlier detector. | |||
| #[wasm_bindgen(constructor)] | |||
| pub fn new( | |||
| detectorType: OutlierDetectorType, | |||
| #[allow(non_snake_case)] detectorType: OutlierDetectorType, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use js_name to preserve Rust naming conventions
Instead of suppressing the non_snake_case lint with #[allow(non_snake_case)], consider renaming the parameter to detector_type to follow Rust naming conventions, and use #[wasm_bindgen(js_name = "detectorType")] to match the JavaScript naming.
Apply this diff to implement the suggestion:
- #[allow(non_snake_case)] detectorType: OutlierDetectorType,
+ #[wasm_bindgen(js_name = "detectorType")] detector_type: OutlierDetectorType,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow(non_snake_case)] detectorType: OutlierDetectorType, | |
| #[wasm_bindgen(js_name = "detectorType")] detector_type: OutlierDetectorType, |
| /// The timestamps of the time series. | ||
| /// | ||
| /// These should be in seconds since the epoch. | ||
| #[tsify(type = "TimestampSeconds[] | BigInt64Array")] | ||
| pub ds: Vec<TimestampSeconds>, | ||
|
|
||
| /// The time series values to fit the model to. | ||
| #[tsify(type = "number[] | Float64Array")] | ||
| pub y: Vec<f64>, | ||
|
|
||
| /// Optionally, an upper bound (cap) on the values of the time series. | ||
| /// | ||
| /// Only used if the model's growth type is `logistic`. | ||
| #[tsify(optional, type = "number[] | Float64Array")] | ||
| pub cap: Option<Vec<f64>>, | ||
|
|
||
| /// Optionally, a lower bound (floor) on the values of the time series. | ||
| /// | ||
| /// Only used if the model's growth type is `logistic`. | ||
| #[tsify(optional, type = "number[] | Float64Array")] | ||
| pub floor: Option<Vec<f64>>, | ||
|
|
||
| /// Optional indicator variables for conditional seasonalities. | ||
| /// | ||
| /// The keys of the map are the names of the seasonality components, | ||
| /// and the values are boolean arrays of length `T` where `true` indicates | ||
| /// that the component is active for the corresponding time point. | ||
| /// | ||
| /// There must be a key in this map for each seasonality component | ||
| /// that is marked as conditional in the model. | ||
| #[tsify(optional)] | ||
| pub seasonality_conditions: Option<HashMap<String, Vec<bool>>>, | ||
|
|
||
| /// Optional exogynous regressors. | ||
| #[tsify(optional, type = "Map<string, number[] | Float64Array>")] |
There was a problem hiding this comment.
Handle Optional Fields in TrainingData Carefully
The added optional fields cap, floor, seasonality_conditions, and x in TrainingData need to be properly validated. Ensure that when these fields are Some, they have the correct lengths matching ds and y. This will prevent runtime errors due to mismatched data lengths.
| yhat: make_forecast(level, value.yhat), | ||
| trend: make_forecast(level, value.trend), | ||
| cap: value.cap, | ||
| floor: value.floor, | ||
| additive: (level, value.additive).into(), | ||
| multiplicative: (level, value.multiplicative).into(), | ||
| additive: make_forecast(level, value.additive), | ||
| multiplicative: make_forecast(level, value.multiplicative), | ||
| holidays: value | ||
| .holidays | ||
| .into_iter() | ||
| .map(|(k, v)| (k, (level, v).into())) | ||
| .map(|(k, v)| (k, make_forecast(level, v))) | ||
| .collect(), | ||
| seasonalities: value | ||
| .seasonalities | ||
| .into_iter() | ||
| .map(|(k, v)| (k, (level, v).into())) | ||
| .map(|(k, v)| (k, make_forecast(level, v))) | ||
| .collect(), | ||
| regressors: value | ||
| .regressors | ||
| .into_iter() | ||
| .map(|(k, v)| (k, (level, v).into())) | ||
| .map(|(k, v)| (k, make_forecast(level, v))) |
There was a problem hiding this comment.
Ensure Accurate Mapping of Forecast Components
When mapping forecast components such as holidays, seasonalities, and regressors, ensure that the make_forecast function is applied correctly with the appropriate level. Any mismatch could lead to incorrect interval calculations in the final Predictions.
| let delta = Float64Array::new_with_length(params.delta.len() as u32); | ||
| delta.copy_from(¶ms.delta); | ||
| let beta = Float64Array::new_with_length(params.beta.len() as u32); | ||
| beta.copy_from(¶ms.beta); |
There was a problem hiding this comment.
Check for Potential Panics in Array Operations
When creating Float64Array instances and copying data, ensure that the lengths match and that there are no out-of-bounds accesses. Adding explicit checks or handling potential errors can prevent runtime panics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
bacon.toml (1)
64-72: LGTM! Consider using a more maintainable pattern for JS package exclusions.The added exclusions correctly prevent doc-testing of the new JavaScript packages, which is consistent with how other JS packages are handled in this file.
To improve maintainability as more JS packages are added, consider using a pattern-based exclusion. For example:
[jobs.doc-test] command = [ "cargo", "test", "--doc", "--color", "always", "--all-features", "--workspace", - "--exclude", "augurs-changepoint-js", - "--exclude", "augurs-clustering-js", - "--exclude", "augurs-core-js", - "--exclude", "augurs-dtw-js", - "--exclude", "augurs-ets-js", - "--exclude", "augurs-mstl-js", - "--exclude", "augurs-outlier-js", - "--exclude", "augurs-prophet-js", - "--exclude", "augurs-seasons-js", - "--exclude", "augurs-js", + "--exclude", "*-js", "--exclude", "pyaugurs", ]This would automatically exclude all JavaScript packages without requiring manual updates for each new package.
README.md (1)
35-35: Consider adding documentation about the modular structure and current limitations.To help users understand the new package structure, consider adding:
- A subsection describing which JS package to use for specific functionality
- Information about potential bundle size improvements
- Known limitations regarding type compatibility across packages
This would help users make informed decisions about which packages to import.
Example addition:
| [`js/*`][js-libs] | WASM bindings to augurs | alpha | +### JavaScript Packages + +The JavaScript bindings are split into separate packages to allow importing only the required functionality: +- `@augurs/changepoint`: Changepoint detection +- `@augurs/clustering`: Time series clustering +[...] + +This modular structure helps reduce bundle size by allowing selective imports. Note: There are currently +known limitations with type compatibility across packages. We're investigating solutions using WASM +components.Also applies to: 86-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
README.md(2 hunks)bacon.toml(1 hunks)js/justfile(1 hunks)js/package.json.tmpl(1 hunks)justfile(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- js/justfile
- js/package.json.tmpl
🔇 Additional comments (4)
justfile (3)
4-12: LGTM! Command simplification improves maintainability
The changes effectively simplify the JS-related commands while maintaining the correct dependency chain. The cleanup step before build is a good practice to prevent stale artifacts.
30-38: LGTM! Complete coverage of JS package exclusions
The test exclusions have been properly updated to accommodate all the new split JS packages, including the previously missing augurs-outlier-js. This aligns well with the PR's objective of modularizing the JS packages.
43-58: LGTM! Well-documented and consistent exclusions
The doctest configuration is well-documented with clear explanations for the exclusions. The consistency with test-all exclusions is a good practice that helps maintain the codebase.
README.md (1)
35-35: LGTM! Table entry update aligns with the modular package structure.
The change from augurs-js to js/* accurately reflects the new split package structure, enabling individual WASM module imports.
This enables us to package up each piece of functionality as a separate WASM module, which means users can import only the functionality they need to save on bundle size.
It doesn't work as-is because some types are expected to work across packages but they're technically considered distinct types after compilation. It's possible that doing this using WASM components and JCO will achieve this but I'm not certain.
Summary by CodeRabbit
New Features
DbscanClustererandChangepointDetectorwith new methods and improved input handling.Bug Fixes
Documentation
Chores
justfilefor better maintainability.