jdb78
[BUG] TimeseriesDataset _set_target_normalizer no longer functions as expected for a setter function.
Describe the bug Latest release has breaking changes for people building extensions to TimeseriesDataset. Because one of the internal setting fuctions no longer behaves as would be expeceted.
The function _set_target_normalizer() now returns the normalizer rather than setting the underlying attribute. GIven the name of the function and how all the other set_something functions work, this is a bit of an antipattern.
Hey @arizzuto
This was intentionally updated recently as you can see we are now setting the attribute target_normalizer through: https://github.com/sktime/pytorch-forecasting/blob/e87230bfe212561f49a38dcb09492e1440c5e0b2/pytorch_forecasting/data/timeseries.py#L570
Any specific example where this change breaks your desired implementation?
About the type hints, some methods don't have those. I am going to open up a PR, adding those😊
Latest release has breaking changes for people building extensions to TimeseriesDataset. Because one of the internal setting fuctions no longer behaves as would be expeceted.
@arizzuto, sorry to hear that!
Generally, it is good practice to build extensions only using public methods. private methods, e.g., that is, the ones starting with _, can change at any moment's notice. This is common convention in python.
Now I also recognize that the current TimeseriesDataset is not convenient for extenders! In fact, very minimally so, we also noticed that. So I completely understand why you wrote a hack-extension to fit your requirements.
The lack of extensibility and customizability is one of the primary reasons why we want to rework the API, making extension and reuse much easier, while remaining downwards compatible as possible with the public interfaces. See here: https://github.com/sktime/pytorch-forecasting/issues/1736 (In fact, @jdb78 had this on the roadmap for long as well, but did not have the time)
Your particpiation and pointers, from the experience of trying to extend and modify, would be much appreciated! E.g., what extension did you try to write, and what are your requirements?
Hi @fkiraly,
totally understand, I am digging into the cogs more than intended. Having said that, keeping the setting in the setter function is also generally common convention in python and elsewhere. It's a minor point overall though, and an easy fix on the extenders side.
As far as my use case goes, I've written a TimeSeriesDataSet extension that can take actuals on the encoder side and forecasts on the decoder side (as is often the case in many real world contexts with exogenous variables) and handles the mapping of prediction time index to the appropriate forecast.
As far as my use case goes, I've written a TimeSeriesDataSet extension that can take actuals on the encoder side and forecasts on the decoder side (as is often the case in many real world contexts with exogenous variables) and handles the mapping of prediction time index to the appropriate forecast.
Interesting - would you be able to share in a draft PR so we can analyze the code and suggest ways to include it in the rework?
Interesting - would you be able to share in a draft PR so we can analyze the code and suggest ways to include it in the rework?
I'll try and find some time to put it together as a PR, the current version is very specific to my use case and doesn't account for most of the options/toggles when creating a timeseriesDataset.