Looks decent, all makes sense

The testing of forecasting via reduction is cool too

A part of me I don't like is thinking to abstract to a ...Predictor class, with fit/predict that Classifier/Regressor extend, but the rest of me says there's nothing to gain and it'd be a waste of time.

Like an MLPPredictor that MLPClassifier and MLPRegressor extend? Maybe it could work using an MLPPredictorMixin (like the mixin classes of sklearn). I’ll have a look, it might be a neat way to handle the network’s parameters.

All looks good to me, the only thing I'm not sure about is the folder structure, networks/deeplearning, we don't want users to access anything is this folder, so perhaps rename it to base/estimators/deeplearning/, that way we can also refactor the classifiers in the sktime base repo into regressors simply by adding more folders to base/estimators/, what do you think?

Folder structure: yes that could work. It would look like:

sktime_dl/base/estimators/deeplearning
sktime_dl/classifiers/deeplearning
sktime_dl/regressors/deeplearning

and in sktime
sktime/base/estimators
sktime/classifiers containing /distance_based and other folders
sktime/regressors add new folder /distance_based and other folders

Seems like a lot of folder name duplications. But perhaps still better than the alternatives. One alternative would be to follow scikit-learn and abandon the classifiers/regressor folder divide and go with a category which contains both classifiers and regressors. Not sure what a good category would be, though. Perhaps supervised or supervised_learning? We would then have files for the sub-categories (deep learning, distance-based, etc) in which both regressors and classifiers (and their shared base/parent classes) live. What do you think?

Yes, the sklearn folder structure might work well. In sklearn there is a file:
sklearn/base.py – contains base classes the different types of estimator:
BaseEstimator
ClassifierMixin
RegressonMixin
ClusterMixin
etc

Then there are separate folders under sklearn for each type of algorithm

• sklearn/neighbors (for KnearestNeighbor) contains
  • _base.py – classes NeighborsBase, KNeighborsMixin, etc
  • _classification.py for class KneighborsClassifier and the classification functions
  • _regression.py for class KneighborsRegressor and the regression functions
• sklearn/neural_network (MLP)
• sklearn/svm
  etc

sklearn-dl could be simililarly structured with

• sklearn-dl/base/estimators containing
  • classifier.py for class BaseDeepClassifier
  • regressor.py for class BaseDeepRegressor
  • cluster.py – add later for unsupervised learning clustering
  • other types of learning e.g. association, reinforcement

and then the architectures go in folders like

• sklearn-dl/deeplearning/cnn
  • _base.py – class CNNMixin to build the core CNN network, minus the final output layer
  • _classifier.py – class CNNClassifier to build_model(), to add the final layer to the CNN, and fit()
  • _regressor.py – class CNNRegressor to build_model() and fit()
  • _cluster.py – add later
  • other types of learning e.g. association, reinforcement
• sklearn-dl/deeplearning/mlp
  • _base.py class MLPMixin
  • _classifier.py
  • _regressor.py
  • _cluster.py – add later

This would keep all CNN code in one place, not split over 3+ folders.

In fact, the deeplearning level could be dropped, since sktime-dl implies deeplearning, leaving

• sklearn-dl/base/estimators (for classifier, regressor, cluster, etc)
• sklearn-dl/cnn
• sklearn-dl/mlp
• folders for other architectures – resnet, fcn, etc

The structure above is like sklearn but is less like the sktime structure (where the distance_based folders live).

Let me know what you think.

Yes, for sktime-dl this works well, happy if you push this on this PR!

For sktime we have to think about it again, as we have additional stuff that is shared across classification and regression modules (composition, tuning, etc) but giving up the classification and regression categories does not seem right.

Work in progress.....

I've thought about the folder structure for base sktime, let me know what you think: #229

Thanks for the invitation to look at #229 . It seems there are strong arguments for sticking with the current folders for classifiers, regressors, etc. Therefore, you could put them as they are in an estimators folder.

The other shared code that you mention is specific to tasks so could go in a tasks folder. Does this task code need to add functionality to each estimator? It might be appropriate to use the Decorator design pattern.

sktime 
└───estimators
|   |───base  	(e.g.for code shared by XXClassifier and XXRegressor)
|   |───classifiers
|   |───regressors
└───tasks  (common to different algorithms)
    └───event_modeller
    └───forecaster
    │   │───base.py
    │   │───composer.py
    │   │───model_selector.py
    └───predictor
    └───segmentor
    └───transformer

Failing on TF 1.15 with "tensorflow.python.framework.errors_impl.InvalidArgumentError: Tensor input_8:0, specified in either feed_devices or fetch_devices was not found in the Graph"

Thanks for your comments!

Forecasters are estimators like classifiers or regressors, and so would be segmentors or event modellers (i.e. probabilistic forecasters). While any regressor can be used to solve a forecasting task (via reduction), there are also specific forecasting algorithms which we want to include (e.g. exponential smoothing). So decorating regressors with forecasting functionality wouldn't be enough, no? Currently, we're encapsulating reduction from forecasting to regression as a meta-forecaster that works with any regressors (see this #218 for work in progress).

You suggest to group by function (i.e. estimation, model selection, etc.), rather than task/setting, so if we include forecasters in estimators, this could look like this:

sktime 
└───estimators
|   |───base  	(e.g.for code shared by estimators)
|   |───classifiers
|   |───regressors
|   |───forecaster
|   └───...

But then I'm not sure how to organise model selection and other composition methods specific to the task/setting (e.g. a SlidingWindowSplitter used when evaluating forecasting models but not useful for evaluating other performance on other tasks).

I also don't like the idea of giving up the classifier and regressors folder on the top level and putting it inside some much more obscure "series_as_features" module, but can't think of anything better at the moment.

Scatter brain/rambling/not fully formed thoughts ahead, sorry

Ultimately there's going to be redundancy somewhere in the structure most likely, question is what redundancy do you want to accept.

In what ways can these things reasonably be grouped?

1. Input/Output
   1a. series -> series transformers
   1b. series -> vector transformers
   1c. series -> double/vector predictors
   1d. model -> model meta processes (composition, tuning, etc)
   1e. a -> b general/other utilities
   1f. etc.

1.2 Output only?

2. High level data tasks
   2a. prediction (classification, regression, forecasting, etc.), aka predictors, estimators, models etc.
   2b. composition
   2c. selection
   2d. tuning
   2e. representation/transformation
   2f. evaluation
   2g. visualisation?
   etcetc.

3. Prediction tasks (basically the members of 2a)
   3a. classification
   3b. regression
   etc.

4. Conceptual algorithm type
   3a. shapelet vs interval vs dictionary etc
   3b. classical vs modern?

Others?

4 is certainly the weakest (or maybe most niche?) grouping IMO, but parties want it in. I'd argue for 2 being the strongest. There's the question of whether prediction (or whatever name it could be) should be an encapsulating folder, or instead if classification,regression... should be their own top level folders, but yeah you need somewhere to put base.py for estimators so it makes sense for the prediction containing folder to be there really.

Then when it comes to which redundancy to accept in relation to e.g. specific evaluation methods as per example above, you probably repeat the classification,regression... categories in each of composition, selection etc. as needed

sktime/
    prediction/
        base.py
        classification/
             base.py
             dictionary_based/
                  boss.py
             ...
        regression/
            base.py
            dictionary_based/
                ...
            ...
         ...
    evaluation/
         base.py 
         ...
         classification/ 
              classification_evaluation_methods.py  # only if needed beyond base/shared methods
         forecasting/
              sliding_window_splitter.py
              ...         
         ...
    composition/
         base.py 
         ...
         classification/ 
              classification_composition_methods.py  # only if needed beyond base/shared methods
         ...

If you argue for 3 being the strongest grouping over 2, you flip the priority of classification,regression etc and put that on top level, while duplicating prediction,evaluation,composition directories

In terms of usage, the imports for some experiment you want to run would then be something like

from sktime.prediction.forecasting import FancyForecaster
from sktime.evaluation import SomeGenericEvalMetric
from sktime.evaluation.forecasting import SlidingWindowSplitter 
...

and so on. From the point of view of a user, the redundancy might not even be that unattractive really, makes it obvious what methods can be shared between prediction tasks and what are specific.

So long as you ignore any code that might be useful for more than one but not all prediction tasks...

Thanks, I like that structure too! One issue is that classification and regression (and to some extent clustering) share composition and tuning functionality, so it doesn't make much sense to put this into composition/classification, so we need some series_as_features folder, no? I don't like the idea of giving up classification/regression grouping either but can't think of any better grouping.

Anyway, before we go on here, would you mind posting that into the other issue thread? @Withington please feel free to comment on the other issue as well!

This seems like a good time to merge this pull request. Seven regressors have been created. The outstanding three are MCDCNN, MCNN and TWIESN. I can work on these but they require a bit more attention:

MCDCNN

• investigate the intermittent failure. “ValueError: Error when checking target: expected dense_9 to have shape (1,) but got array with shape (2,)”
• Enable multi-variate use. Add multi-variate tests before making code changes to create the regressor.

MCNN

• add a test on a dataset where dl-4-tsc achieves good accuracy. On ItalyPowerDemand only 0.5 is achived by both dl-4-tsc and sktime-dl.

TWIESN

• investigate why sktime-dl accuracy on ItalyPowerDemand is only 0.49 while dl-4-tsc achieved 0.88.
• add a test on a dataset where sktime-dl acheives good accuracy.

Could you please review the current pull request and consider merging it to dev?

There are now a lot of changes so feel free to ask me about it. I kept it fairly clean by moving entire blocks of code from name/_classifier.py to name/_base.py.

Comments above. Please hit the "Resolve conversation" button if my replies cover your points satisfactorily.

I'm happy. Nice work @Withington. Added some comments to the individual queries above, I reckon everything's fine as is. Shall we get this into dev and then look into the dev branch TS problems if they still persist?

Will leave this up for another couple hours or so while I head home in case of any sudden changes of heart, and merge it in

Thanks for the merge! 😃

What are the "dev branch TS problems" @james-large ? I'm not sure what TS is.

Ah meant tf, tensorflow. There was some problem with the tf2.x version of the travis checks iirc? No matter, looks fixed, maybe it was long ago or I imagined it. Brain's still recovering

TF 2.0 was fine, that is in dev branch now.
TF 2.1 was a problem:

• TensorFlow 2.1 and scikit-learn 0.22 - build fails with "It seems that scikit-learn has not been built correctly."
• TensorFlow 2.1 and scikit-learn 0.21.3 works fine, the build passes.
  I'll try again when there have been some updates.