-
Notifications
You must be signed in to change notification settings - Fork 487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Minor] fix future regressor #1585
[Minor] fix future regressor #1585
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1585 +/- ##
=======================================
Coverage 88.16% 88.16%
=======================================
Files 41 41
Lines 5374 5366 -8
=======================================
- Hits 4738 4731 -7
+ Misses 636 635 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added three comments, please check
neuralprophet/forecaster.py
Outdated
@@ -1107,7 +1112,7 @@ def fit( | |||
self.fitted = True | |||
return metrics_df | |||
|
|||
def predict(self, df: pd.DataFrame, decompose: bool = True, raw: bool = False): | |||
def predict(self, df: pd.DataFrame, decompose: bool = True, raw: bool = False, auto_extend=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid the linked issues being raised, auto_extend
should default to True
, unless my logic is off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto_extend for me means that we don't do the cutting at the end. So we cut all the time except for the case where it will cause the whole dataframe to be nan. Can still reverse it
neuralprophet/forecaster.py
Outdated
@@ -749,6 +749,7 @@ def add_events( | |||
def add_country_holidays( | |||
self, | |||
country_name: Union[str, list], | |||
subdivision_name: Optional[Union[str, dict]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like the subdivision commits slipped in here.
we can either remove those commits or merge that PR first and be careful that these lines do not longer reappear due to commit squashing.
neuralprophet/configure.py
Outdated
@@ -397,12 +397,12 @@ def __post_init__(self): | |||
self.seasonality_local_reg = False | |||
|
|||
# If seasonality_local_reg = True | |||
if self.seasonality_local_reg == True: | |||
if self.seasonality_local_reg: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also seems to come from another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this change will always override a set seasonality_local_reg
and overwrite it to be 1
which does not appear intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No was trying to solve the flake8 errors - doesn't make any difference anyways
…/neural_prophet into bug/1567-fix-future-regressor
Model Benchmark
\n
Model training plots\n ## Model Training ### PeytonManning ![](https://asset.cml.dev/3f1106eec6cbcdc027eba6be6facdb1ca27fa1fe?cml=svg%2Bxml&cache-bypass=c347c940-7263-4346-89b1-b0c25aa295a9) ### YosemiteTemps ![](https://asset.cml.dev/e8f030afe97e92f42a15a8070d163936ad994061?cml=svg%2Bxml&cache-bypass=a71c29e9-1d3c-4fba-a996-fffb3464ca9b) ### AirPassengers ![](https://asset.cml.dev/49860d361e5e48376530bc11ae75aa2703fdbd7d?cml=svg%2Bxml&cache-bypass=62e614dc-cec9-4eb0-99db-0ef0dcf9a3ed) ### EnergyPriceDaily ![](https://asset.cml.dev/ea307c95649c9afd52a11529f825463e73d39a7c?cml=svg%2Bxml&cache-bypass=d3b78574-5f3b-416b-8a2c-889d3f453ccb) \n |
🔬 Background
🔮 Key changes
📋 Review Checklist
Please make sure to follow our best practices in the Contributing guidelines.