feat: add Forecaster wrapper for Prophet #191
Conversation
Since the `Forecaster` traits don't pass timestamps etc, we need to provide the training data at the time the forecaster is created and pass _that_ down to the Prophet model, after replacing the `y` column with whatever the forecaster gives us. This should work with transforms too. It does require cloning the data, unfortunately. The alternative approach which I tried first was to use an associated type for the data, but that is pretty infectious and ends up requiring the input data to be mutable so that we can replace the `y` column, which ends up looking pretty ugly.
|
Warning Rate limit exceeded@sd2k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 25 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. 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request introduce several enhancements to the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
crates/augurs-prophet/src/forecaster.rs (3)
55-59: Consider avoiding unnecessary cloning ofTrainingDataIn the
fitmethod,training_datais cloned to replace theyvalues. For large datasets, this could lead to performance issues. Consider modifyingTrainingDatato allow updating theyvalues without cloning, or refactoring to work with references to minimize overhead.
60-61: Assess the necessity of cloning the modelThe model is cloned before fitting, which might be inefficient if the model is large. If possible, refactor to avoid cloning the model by leveraging interior mutability or other patterns to modify the model in place.
95-97: HandleOptionwithout usingexpectto prevent potential panicsIn
predict_in_sample_inplaceandpredict_inplace, the code usesexpectonOptionvalues forpredictions.yhat.lowerandpredictions.yhat.upper. Relying onexpectmay lead to panics if the assumption aboutuncertainty_samplesbeing greater than zero is ever violated. Consider gracefully handling theOptionusing pattern matching or providing a fallback to improve robustness.Also applies to: 134-137
examples/forecasting/examples/prophet_forecaster.rs (1)
12-15: Improve readability of timestamp dataThe
dsvector contains hardcoded UNIX timestamps, which might not be immediately clear to readers. Consider using date parsing functions or adding comments to clarify the dates represented, enhancing the example's readability.crates/augurs-prophet/src/optimizer.rs (1)
302-311: Consider adding documentation for the Arc implementationAdding documentation would help explain the purpose of this implementation, particularly its role in enabling thread-safe sharing of optimizers.
Consider adding documentation like this:
+/// Implementation for Arc<dyn Optimizer> enables thread-safe sharing of optimizers +/// while maintaining the ability to use dynamic dispatch. impl Optimizer for Arc<dyn Optimizer> { fn optimize( &self, init: &InitialParams, data: &Data, opts: &OptimizeOpts, ) -> Result<OptimizedParams, Error> { (**self).optimize(init, data, opts) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
crates/augurs-prophet/Cargo.toml(1 hunks)crates/augurs-prophet/src/forecaster.rs(1 hunks)crates/augurs-prophet/src/lib.rs(1 hunks)crates/augurs-prophet/src/optimizer.rs(2 hunks)crates/augurs-prophet/src/prophet.rs(3 hunks)examples/forecasting/examples/prophet_forecaster.rs(1 hunks)
🔇 Additional comments (6)
crates/augurs-prophet/src/prophet.rs (2)
238-256: Accessor methods enhance flexibility of Prophet
The addition of opts, opts_mut, and set_interval_width methods provides users with convenient access to modify ProphetOptions. This enhancement improves the configurability of the Prophet model.
292-322: Dynamic optimizer conversion and forecaster integration are well-implemented
The into_dyn_optimizer method correctly converts the optimizer into a dynamic trait object, facilitating compatibility with trait objects. The into_forecaster method seamlessly integrates the Prophet model with ProphetForecaster, enhancing usability within the forecasting framework.
crates/augurs-prophet/src/lib.rs (1)
5-5: Addition of forecaster module
Introducing the forecaster module exposes the new forecasting capabilities to users, making it straightforward to access the ProphetForecaster functionality from the library's root. This aligns with the library's modular design.
examples/forecasting/examples/prophet_forecaster.rs (1)
1-53: Example effectively demonstrates ProphetForecaster usage
The example provides a clear demonstration of setting up a ProphetForecaster with data transformations and generating predictions. The code is well-structured and serves as a practical guide for users.
crates/augurs-prophet/Cargo.toml (1)
31-31: LGTM: Workspace dependency addition is appropriate
The addition of augurs-core as a workspace dependency aligns with the PR objective of implementing the Forecaster traits.
crates/augurs-prophet/src/optimizer.rs (1)
302-311: LGTM: Clean implementation of dynamic dispatch
The implementation correctly enables thread-safe sharing of optimizers through Arc while maintaining proper delegation semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/augurs-prophet/src/forecaster.rs (3)
32-46: Consider documenting the uncertainty_samples default value.The method correctly ensures uncertainty_samples is at least 1000, but this default value should be documented in the method's documentation.
/// Create a new Prophet forecaster. /// /// # Parameters /// /// - `opts`: The options to use for fitting the model. /// - `optimizer`: The optimizer to use for fitting the model. /// - `optimize_opts`: The options to use for optimizing the model. + /// + /// Note: If uncertainty_samples is 0, it will be set to 1000.
53-66: Consider optimizing data cloning for large datasets.The
fitmethod clones the entire training data. While this ensures data isolation, it could be memory-intensive for large datasets. Consider documenting this behavior and its potential impact on performance.
91-104: Reduce code duplication in uncertainty samples handling.The error message for uncertainty samples is duplicated. Consider extracting this into a constant or helper method.
+ const UNCERTAINTY_SAMPLES_MSG: &str = "uncertainty_samples should be > 0, this is a bug"; // Then use it in both places: - .expect("uncertainty_samples should be > 0, this is a bug"); + .expect(UNCERTAINTY_SAMPLES_MSG);Also applies to: 130-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
crates/augurs-prophet/src/forecaster.rs(1 hunks)crates/augurs-prophet/src/optimizer.rs(2 hunks)
🔇 Additional comments (4)
crates/augurs-prophet/src/forecaster.rs (3)
10-22: LGTM! Well-documented struct with clear purpose.
The struct is well-designed with appropriate fields and good documentation explaining its purpose and relationship with the augurs framework.
69-74: LGTM! Clean struct design with appropriate use of RefCell.
The struct correctly uses RefCell for interior mutability of the model, which is necessary for the predict implementations.
108-145: LGTM! Robust implementation with good error handling.
The predict_inplace method has good error handling, particularly for the zero horizon case, and properly manages the model borrowing scope.
crates/augurs-prophet/src/optimizer.rs (1)
302-314: LGTM! Clean implementation enabling thread-safe optimizer sharing.
The implementation correctly delegates to the inner type while maintaining thread safety through Arc. The documentation clearly explains its purpose.
Since the
Forecastertraits don't pass timestamps etc, we need to provide the training data at the time the forecaster is created and pass that down to the Prophet model, after replacing theycolumn with whatever the forecaster gives us. This should work with transforms too.It does require cloning the data on each call to
fit, but I don't think that's avoidable since Prophet'sfittakes the data by value anyway.The alternative approach which I tried first was to use an associated type for the data, but that is pretty infectious and ends up requiring the input data to be mutable so that we can replace the
ycolumn, which ends up looking pretty ugly.This is an alternative (and hopefully simpler) approach to #184.
Summary by CodeRabbit
New Features
ProphetForecasterfor enhanced forecasting capabilities.forecastermodule for easier access to forecasting functionalities.Prophetmodel to improve usability, including options management and interval width settings.Prophetmodel with theWasmstanOptimizer.Bug Fixes
Documentation
ProphetForecaster.