Refactor TSF classifier into TSF regressor

[luiszugasti]

Reference Issues/PRs

Working on issue #212. Following through with discussion with Ori and Marcus from this Monday, February 18th.

What does this implement/fix? Explain your changes.

Mostly focusing on translating TimeSeriesForest classifier into a regressor.

Does your contribution introduce a new dependency? If yes, which one?

Not yet.

What should a reviewer concentrate their feedback on?

Incremental code improvements.

Any other comments?

None yet.

PR checklist

For all contributions

• (Luis) I've added myself to the list of contributors.
• (Kavin) I've added myself to the list of contributors.

For new estimators existing estimators:

• I've added the estimator to the online documentation.
• I've updated the existing example notebooks or provided a new one to showcase how my estimator works.
• I've updated tests to ensure the API works.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[luiszugasti]

Tagging @uhbuhb and @kanand77 for visibility

[luiszugasti]

Todo:

• Work on our code
• Write testing based on the code/comply with APIs
• Post comments if we get stuck on learning any of the relevant algos

After this, will focus on rebasing to bring the branch in sync with master.

[uhbuhb]

Hey, so first step from here should be to rebase over origin/master or upstream/master, so its possible to see the commit you did, bc all the commits make it hard to find your code.
I just aligned the fork above the upstream master so
git fetch origin && git rebase origin/master && git push -f
should do :)

Good on putting up a PR

[luiszugasti]

Not all tests are passing, we are playing around with sklearn's API for now.

[mloning]

Hi @luiszugasti instead of changing the file in the classification module, copy the code to a new file sktime/regression/interval_based/_tsf.py and make the change there, also change the parent class from BaseClassifier to BaseRegressor - that should get the tests to pass.

[mloning]

You still need to undo the changes in the classification module, reverting that commit or overwriting it with the version on master should do it.

[luiszugasti]

Noted. I made a mistake using git so I reset the HEAD~ in one of my commits where I undid the changes.

[luiszugasti]

Just pushing that for now, am taking a look at specific implementation requirements

[luiszugasti]

Ok so I refactored common functionality to base class. I allowed for some methods that had little/no coupling to be concrete in the base class, but this can always be reimplemented.

In terms of other work, I think the remaining is testing the API for the new classifier, although I think that will be manageable after we discuss in our next meeting.

Apart from that let us know what others we can refactor, @mloning I notice for example _ensemble.py can have a similar structure to this. Hope that the Base*class methodology is acceptable if not look forward to your feedback.

[mloning]

Looks great @kanand77 and @luiszugasti - please mark this PR "ready for review" once you think it's good to go.

There's a simple error now - I think because of the renaming with "classifier", searching and replacing all instances should fix that:

E ImportError: cannot import name 'TimeSeriesForest' from 'sktime.classification.interval_based._tsf'

[luiszugasti]

@mloning I'd like your input on something I uncovered.
With this refactor we created a naming conflict between this model (TimeSeriesForest) and a more "in-depth" TimeSeriesForest.
Basically between the models found in classification.compose vs classification.interval_based.
This leads the test found at utils.tests.test_all_estimators.py>test_all_estimators_return_names test to fail.

I propose one solution: scrap the new name that we chose of "TimeSeriesForestClassifier" and name it something more representative of its use - it's used as a HIVE-COTE component so might as well call it TimeSeriesForestClassifierHiveCote.

I'm pretty sure that shouldn't have a negative effect since we renamed this current usage elsewhere in the tests.

Anyways; I'm going to keep working on writing an API test for this.

[uhbuhb]

seems like this commit should be removed as this change is reverted in the next commit... unless im missing something..

[mloning]

Thanks @luiszugasti - as discussed, looks all good, I'll give it a closer look when I have some more time and take care of the naming conflict of having two TimeSeriesForestClassifiers.

[luiszugasti]

Sounds good @mloning ! Look forward to the outcome.

[mloning]

I've not forgotten about this, just was really busy over the last few days!

[luiszugasti]

No worries @mloning. Not a blocker on my side!

[TonyBagnall]

hi, its Tony Bagnall not Badnall :)

[TonyBagnall]

so we are moving to having estimators/classification/regression packages? What goes in estimators as opposed to classification/regression? It should be estimation surely, since @mloning does not like classifiers and regressors?

edit: misread it, seems estimators is in series_as_features and this was already in. I think the naming point still stands? We are against nouns for packages?

[TonyBagnall]

all looks good to me

[luiszugasti]

@mloning looks like merging is blocked, do you want us to add any more work on our side

[mloning]

@luiszugasti @kanand77 @uhbuhb now merged - thanks for working on this! 🎉