-
Notifications
You must be signed in to change notification settings - Fork 46
Feat/optional training start #1917
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Documentation build overview
Show files changed (5 files in total): 📝 5 modified | ➕ 0 added | ➖ 0 deleted
|
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
…t_timing_parameters_of_forecaster_parameters_schema"`) Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…ameter in expected_timing_output Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
This reverts commit d5c77f1.
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…hema Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…ParametersSchema Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…riod) Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…ncy (predict_period) Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…metersSchema validation Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…f FutureWarning: 'H' is deprecated Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…n forecasting docs. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…Schema. this allows for default values to be loaded Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…n data_add Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Flix6x
left a comment
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.
It's so nice to have these new test cases to lead the discussion! They will provide a lot of clarity, and I imagine they will find there way into the docs, too, at some point.
My remarks right now are mainly ideas to add more clarity on intent and expectations to those test cases.
| @pytest.mark.parametrize( | ||
| ["timing_input", "expected_timing_output"], | ||
| [ | ||
| # Test defaults when no timing parameters are given |
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.
Let's add the (not floored) belief time to the expectations as well.
| "train_period_in_hours": 720, # from start_date to predict_start | ||
| "predict_period_in_hours": 120, # from predict_start to end_date | ||
| # default values | ||
| "max_forecast_horizon": pd.Timedelta(hours=48), |
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 think that as a user I would have expected 5 days here.
What is the retrain frequency? The retrain frequency governs the number of cycles, right? Let's add an expectation for that parameter, too.
| @pytest.mark.parametrize( | ||
| ["timing_input", "expected_timing_output"], | ||
| [ | ||
| # Test defaults when no timing parameters are given |
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.
And let's already mention 3 main characteristics in each test case comment:
- Duration of training period (for 1st cycle)
- Duration of prediction period
- Number of cycles / (unique) belief times we expect.
| "max_forecast_horizon": pd.Timedelta(hours=48), | ||
| "train_period_in_hours": 720, | ||
| "max_training_period": pd.Timedelta(days=365), | ||
| "forecast_frequency": pd.Timedelta(hours=1), |
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 governs how much each horizon is apart, right, rather than the number of cycles.
| "forecast_frequency": pd.Timedelta(hours=1), | ||
| }, | ||
| ), | ||
| # Test when only start date is given with a training period |
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.
Nice. Let's explicitly mention that for this case we expect the predict start to be computed with respect to the start date.
| "forecast_frequency": pd.Timedelta(hours=1), | ||
| }, | ||
| ), | ||
| # Test when only end date is given with a training period |
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.
Nice. Let's explicitly mention that for this case we expect the train start to be computed with respect to (floored) now.
| "forecast_frequency": pd.Timedelta(hours=1), | ||
| }, | ||
| ), | ||
| # Test when only end date is given with a training period |
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.
One more suggested case:
# Test when only end date is given with a prediction period: we expect the train start and predict start to both be computed with respect to the end date.
| "forecast_frequency": pd.Timedelta(hours=1), | ||
| }, | ||
| ), | ||
| # Test when only start date is given with a retrain frequency (prediction period) |
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.
Interesting case. What would the user's intended effect be?
| @pytest.mark.parametrize( | ||
| ["timing_input", "expected_timing_output"], | ||
| [ | ||
| # Test defaults when no timing parameters are given |
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.
Let's also prepend a comment stating the user's intent. For instance, in this case, I just need forecasts within my current planning horizon.
Actually, once having formulated that just now, I immediately recognize that it would make a lot of sense to tie the default prediction window to the app's default planning horizon.
Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Description
start-dateto be set by the userend-dateto be set by the userForecasterParametersSchemadirectly to validate derivation of timing parametersdocumentation/changelog.rstLook & Feel
possible to run forecasting cli command without
start_dateorend_date:Examples:
flexmeasures add forecasts --sensor 20 --start-date "2025-01-01T00:00:00+01"flexmeasures add forecasts --sensor 20 --end-date "2025-01-15T00:00:00+01"flexmeasures add forecasts --sensor 20How to test
pytest flexmeasures/data/schemas/tests/test_forecasting.py...
Further Improvements
train-periodvsmax-training-windowincli.data_add.train_predict_pipeline(and for our first example inforecasting.rstwe should mentiontrain-periodrather thanmax-training-window).def train_predict_pipeline->def add_forecast