Open MarcBresson opened 10 months ago
Hu @MarcBresson and sorry for the late response.
It would indeed be nice to check whether the model in general supports specific features (from the class level):
Some of features change support depending on how the user created the model. That's why for the final model (object) support we still need (at least some of) the properties. An example supports_past_covariates
for RegressionModel
depends on whether the user specified lags_past_covariates
at model creation.
Regarding the proposed soultion. We need to make sure that all models implement this. Adding it as a class variable has the risk, that for some future models we forget to define it.
Another way would be to define new class methods:
# we could make it abstract in `ForecastingModel` to enforce subclasses to implement it
@classmethod
def supports_past_covariates_cls) -> bool:
return True/False
This can be called with RegressionModel.supports_past_covariates_()
.
We'd still have reimplement this for each sub class which can get a bit messy. I wonder too, if there was a more clean way to do this. @madtoinou any idea?
Hey, thank you for reaching back!
We are in an edge case were function signature overloading would be cool to have. Anywho this is not the subject. Your solutions seems quite nice. As for supports_past_covariates
for RegressionModel
, the supports_past_covariates_cls
method would return True
as the model can support it if it is provided with the right arguments.
Nice suggestion, I like the idea.
We need to make sure the API is intuitive to differentiate the supports_...
at the class and instance level. I would maybe call it can_support_...
(instead of support_..._cls
) at the class level, and keep the instance level as it is.
@dennisbader Maybe we should also explore this idea of having a dict listing the properties of a model and have the methods in ForecastingModel
read from it, so that we don't need to re-implement the several abstract methods in several classes.
@madtoinou if we add a dict as a class variable for each model (e.g. under _properties: dict
), some users maybe tempted to fiddle with it. From that constatation, we can:
Lastly, the darts' doc is not clear on the value behind properties like future_supports_covariates
. Having a dict that is read by the (opaque) ForecastingModel
metaclass can mislead a user that is looking for the piece of code that corresponds to supports_future_covariates
. Indeed, the user will likely click on the "source" button of the doc and do a ctrl+f
with the property name. Hence, we must make sure that the dictionary keys have the expected same name as the property so that the user can find it.
It was actually my first time encountering a metaclass in a python library, and it took me a loong time to figure out why some class arguments were not used according to my IDE.
Is your feature request related to a current problem? Please describe. If we want to check if a model supports a feature (multivariate or static covariates), we must initialize it first before having an access to the instance property (e.g.
supports_static_covariates
). I see two small issues with that:Describe proposed solution Python does not support read-only properties available at a class level. One way we circumvent fix that is by adding a duplicate class variable like
TFTModel.supports_multivariate_
that has the same value as the related property. It's probably not the cleanness way but I don't see any other.