[FIX] Unify API #1023
[FIX] Unify API #1023
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…neuralforecast into fix/docs_and_refactoring
|
I ran my own small experiment, and models are slightly faster, with no degradation in performance. The refactoring looks good to me, but a second opinion would be great on this! |
Thanks! |
There was a problem hiding this comment.
i noticed that this file and action_files/test_models/src/evaluation2.py are quite similar. i have a couple of suggestions:
- this might be a good opportunity to use the
utilsforecastevaluation features. we could replacemaeandsmapefrom thelossesmodule and theevaluatefunction. - also, it looks like the only difference between this file and the second one is the list of models, correct? if that’s the case, we could combine them into a single file and use fire to pass the list of models. you could then call it in the
.github/workflows/ci.yamlfile.
the idea would be to abstract the code in the if __name__ == '__main__': clause, something like this:
def main(models: list):
groups = ['Monthly']
datasets = ['M3']
evaluation = [evaluate(model, dataset, group) for model, group in product(models, groups) for dataset in datasets]
evaluation = [eval_ for eval_ in evaluation if eval_ is not None]
evaluation = pd.concat(evaluation)
evaluation = evaluation[['dataset', 'model', 'time', 'mae', 'smape']]
evaluation['time'] /= 60 # minutes
evaluation = evaluation.set_index(['dataset', 'model']).stack().reset_index()
evaluation.columns = ['dataset', 'model', 'metric', 'val']
evaluation = evaluation.set_index(['dataset', 'metric', 'model']).unstack().round(3)
evaluation = evaluation.droplevel(0, 1).reset_index()
evaluation['AutoARIMA'] = [666.82, 15.35, 3.000]
evaluation.to_csv('data/evaluation.csv')
print(evaluation.T)and then you could use fire inside the main clause:
if __name__ == '__main__':
import fire
fire.Fire(main)this way, we can run it for different models inside .github/workflows/ci.yaml: python -m action_files.test_models.src.evaluation --models <list of models>. wdyt?
There was a problem hiding this comment.
this also could apply to action_files/test_models/src/multivariate_evaluation.py. since we are changing models and datasets, we could define main(models: list, dataset: str).
There was a problem hiding this comment.
Good idea, one remarkt - why favour ci over circleci? (I'm ambivalent, don't know why we would prefer one over the other)
There was a problem hiding this comment.
maybe we could also merge action_files/test_models/src/models.py, action_files/test_models/src/models2.py, and action_files/test_models/src/multivariate_models.py. from what i can tell, the only real difference between them is the list of models. if that’s the case, we could add a new parameter to main—maybe config: str or something similar—and then have a dictionary with different models based on the config. for example: {"multivariate": , ...}, and then we could call it like this: python -m action_files.test_models.src.models --config multivariate.
let me know what you think.
There was a problem hiding this comment.
Makes sense, then we can still fire up multiple runners (for the sake of keeping test time under control it makes sense to split the tests)
| # from neuralforecast.models.rnn import RNN | ||
| # from neuralforecast.models.tcn import TCN | ||
| from neuralforecast.models.lstm import LSTM | ||
| from neuralforecast.models.dilated_rnn import DilatedRNN | ||
| from neuralforecast.models.deepar import DeepAR | ||
| from neuralforecast.models.mlp import MLP | ||
| from neuralforecast.models.nhits import NHITS | ||
| from neuralforecast.models.nbeats import NBEATS | ||
| # from neuralforecast.models.deepar import DeepAR | ||
| # from neuralforecast.models.mlp import MLP | ||
| # from neuralforecast.models.nhits import NHITS | ||
| # from neuralforecast.models.nbeats import NBEATS |
There was a problem hiding this comment.
is the commented code going to be restored in the future? if this change is permanent, maybe we could delete those lines instead.
There was a problem hiding this comment.
I'm kind of treating this file also as a testing file locally, we can delete it (it's mainly so that testing locally is faster that you don't need to type all that stuff every time)
| " n_series = 1\n", | ||
| " self.n_series = n_series \n", |
There was a problem hiding this comment.
should we set n_series = None in this case? using n_series = 1 might lead to unexpected bugs, though i'm not entirely sure if that's the case here.
There was a problem hiding this comment.
We need n_series=1 for the univariate models in a few other places, but I can also introduce additional helper variables to deal with that situation, but that feels a bit unnecessary perhaps....
This is a large refactoring PR and open for discussion. The main goal of the PR is to unify API across different model types, and unify loss functions across different loss types.
Refactoring:
BaseWindows,BaseMultivariateandBaseRecurrentintoBaseModel, removing the need for separate classes and unifying model API across different model types. Instead, this PR introduces two model attributes, yielding four possible model options:RECURRENT(True/False) andMULTIVARIATE(True/False). We currently have a model for every combination except a recurrent multivariate model (e.g. a multivariate LSTM), however this is now relatively simple to add. In addition, this change allows to have models that can be recurrent or not, or multivariate or not on-the-fly, based on users' input. This also allows for easier modelling going forward.domain_mapfunctions.loss.domain_mapoutside of models toBaseModelTSMixer,TSMixerxandRMoKtocommon.modulesFeatures:
DistributionLossnow supports the use ofquantilesinpredict, allowing for easy quantile retrieval for all DistributionLosses.GMM,PMMandNBMM) now support learned weights for weighted mixture distribution outputs.quantilesinpredict, allowing for easy quantile retrieval.ISQFby adding softplus protection around some parameters instead of using.absBug fixes:
MASEloss now works.StudentTincrease default DoF to 3 to reduce unbound variance issues.eval: falseon the examples whilst not having any other tests, causing most models to effectively not being tested at all.Breaking changes:
input_sizeto be given.TCNandDRNNare now windows models, not recurrent models.Tests:
common._model_checks.pythat includes a model testing function. This function runs on every separate model, ensuring that every model is tested on push.Todo: