sktime icon indicating copy to clipboard operation
sktime copied to clipboard

[ENH] Migrating Estimators from sktime-dl

Open AurumnPegasus opened this issue 2 years ago • 16 comments

Is your feature request related to a problem? Please describe. Related to the task of migrating estimators from sktime-dl to sktime. The main goal of doing so is to have all networks/models in a single package, following similar structure and quality.

Steps for Migration

Choose one of the Classifiers or Regressors from the list below, and create an issue for it. There are 2 key components to a DL Estimator, the Network (creates the actual code of neural network) and the Estimator (Is a layer of abstraction to make the user's life easier). Lets say I want to create CNNClassifier (steps are similar for any other DL Regressor or Classifier, whereever there are differences, I will mention them)

  1. Our first task would be to check if the network has already been implemented or not. Networks are often common for multiple estimators (for eg, CNNClassifier and CNNRegressor use the same CNNNetwork). So, I would check if a file named cnn.py exists in sktime/sktime/networks. If it does, that means that the network has already been made, and you can skip to the step to create the classifier.
  2. If the network has not been created, we need to create one. Create a file called cnn.py in sktime/sktime/networks. This file will contain our CNNNetwork. You will have to migrate the network from sktime-dl to sktime. To do so, you will need to go to sktime-dl/networks and look for _cnn.py
  3. Migrate this code from sktime-dl to sktime. An example of this migration is that the original code of _cnn.py from sktime dl has been migrated as cnn.py in sktime. The code, for most part, remains the same, with different documentation at most. There are a few cases where the migration is not as straightforward, and it would be described in the end.
  4. Ensure all the tests pass for the Network you created! Run the pytest, and then run the check_estimator tests as mentioned in the developer guide. Once all the tests pass, you have successfully migrated your Network.
  5. Now, to create an estimator, lets create a new file called cnn.py at sktime/sktime/classification/deep_learning/. You will be migrating the CNNClassifier here. You will have to migrate the network from sktime-dl to sktime. To do so, go to sktime-dl/classification and look for _cnn.py.
  6. Migrate this code from sktime-dl to sktime. An example of this migration is that the original code of _cnn.py from sktime-dl has been migrated as cnn.py in sktime. The code and core logic, for most part, remains the same. There are minor differences in _fit method you will need to take care of. For other cases where the migration is not as straightforward are described in the end.
  7. Ensure all the tests pass for the Estimator you created! (the steps here are the same as step 4, but for Classifier/Regressor instead of Network)

Special Cases

Adding Soft Dependancy
  1. In cases where the particular network/estimator requires an additional dependency, (for example, CNTCNetwork requires keras_self_attention), you have to add them as a soft dependancy
  2. Firstly, go to pyproject.toml, and check in the all_extras list whether the dependency you want to add (lets say keras_self_attention), already exists within it or not. IF it does, it means that it already is a soft dependancy, and you can skip the steps required to add it as a soft dep.
  3. To add a dependency as soft dependency, follow the instructions mentioned in the dev guide. You mostly will have to add it in the all_extras list of pyproject.toml.
  4. Next, we need to go to the classes where we need to use the soft dep, and add a check for _check_soft_dependencies. An example for how it is done is there in CNTCNetwork

DL Classifiers:

DL Regressors:

AurumnPegasus avatar Aug 26 '22 12:08 AurumnPegasus

quick question about the recipe: do we always need to create a separate network? Or are there a few common ones? E.g., for regression/classification? Should the regressor/classifier not use the same network?

fkiraly avatar Aug 26 '22 12:08 fkiraly

Yup, made those changes. Will add more instructions as well

AurumnPegasus avatar Aug 26 '22 13:08 AurumnPegasus

I want to work on the implementation of ResNet Classifier and regressor, but i could not understand the migration steps. @fkiraly @AurumnPegasus can you please help me.

nilesh05apr avatar Aug 29 '22 00:08 nilesh05apr

Hey @nilesh05apr , Maybe the steps are not clear enough, I will work on it to improve it. In the meanwhile:

Firstly, create an issue for the estimator you want to work on. Then, There are 2 main components to a DL estimator: the Network, and the Classifier (or Regressor). For ResNet, as you can see in this issue itself, the Network has not been migrated yet.

So firstly, go to the network code in sktime-dl. This is the code you need to migrate to sktime. Create a file in sktime/sktime/networks/ called as resnet.py. Within that, migrate the code from the code base to resnet.py according to examples provided above (like in #3232 ) Once the main network has been created, you can create the respective estimators from it.

To create the Classifier, go to classifier code in sktime-dl. This is the code you need to migrate to sktime. Create a file in sktime/sktime/classification/deep_learning called resnet.py. Within that, migrate the code from the code base to resnet.py according to examples provided above (line in #3232 ).

You can import the documentation following the way it has been written in either MLPClassifier or CNNClassifier/CNNRegressor.

I hope it helps! Let me know if you get stuck somehwere.

AurumnPegasus avatar Aug 29 '22 05:08 AurumnPegasus

Thanks @AurumnPegasus for the clarification, it's much clear now. I have started working on it.

nilesh05apr avatar Aug 29 '22 21:08 nilesh05apr

@AurumnPegasus @fkiraly i have made a pr will resnet implemented can you please review it and also assign me this issue. i will migrate all the estimators from this sktime_dl to sktime.

nilesh05apr avatar Aug 31 '22 12:08 nilesh05apr

@fkiraly I have updated the wordings and migration instructions, hope they are better. I will add the code and examples for special cases in some time (since we have not really agreed to a couple of cases yet). Also, would request you to add the good first issue label to this.

AurumnPegasus avatar Aug 31 '22 12:08 AurumnPegasus

@AurumnPegasus can you please link steps to test the changes locally.

nilesh05apr avatar Aug 31 '22 13:08 nilesh05apr

Hey @nilesh05apr , Ensure that you have all the developer requirements installed, follow the instructions mentioned here

Once that is done, to test the estimators (lets say you want to test ResNetClassifier) Firstly, run all the pytests (you can do it via vscode as well) via the general bash command: pytest ./sktime/classification/

Then, run the check_estimators test as mentioned here

Hope it helps!

AurumnPegasus avatar Aug 31 '22 13:08 AurumnPegasus

I'll work on TapNet Classifier Migration. Do add #3372 to the list.

achieveordie avatar Sep 01 '22 07:09 achieveordie

Hey, taking up one regressor model as well. Kindly add #3474 to the list.

achieveordie avatar Sep 24 '22 07:09 achieveordie

Hi there!!! This is Arushika, I am quite new to open source. I have been wanting to implement RNN regressor but (as per the steps you specified), I will need a "_rnn.py" file in sktime-dl/networks but as it turns out there is no network of that sort So what am I supposed to do in this case Should I myself write a _rnn.py file????

ArushikaBansal avatar Jan 21 '23 06:01 ArushikaBansal

Hey @ArushikaBansal, thanks for taking this up. In the specific case of RNNRegressor it looks like the underlying Network model would be relatively small. That may be why the original author wrote it in a single file. But if we were to port this into sktime, we'd want it a separate Network file since we want to keep the API design consistent.

You can use this file to create both the network and the estimator so it shouldn't be much of trouble. In case you face any difficulty, feel free to ask me or any other core dev you want to ask.

(Do keep in mind to use one of the already ported estimators as a reference, that will make things easier.)

achieveordie avatar Jan 21 '23 16:01 achieveordie

Thankyou sir for the help!!!!! I will try doing the same.

ArushikaBansal avatar Jan 21 '23 17:01 ArushikaBansal

I made the required changes please review them .

ArushikaBansal avatar Feb 01 '23 09:02 ArushikaBansal

FYI, for all contributors to sktime-dl estimators.

  • we are wrapping up migration of remaining sktime-dl estimators to sktime
  • sktime has adopted an estimator level package management approach, with owners and dependencies specified via scikit-base tags

Issues will also be migrated to sktime, and a migration of unreviewed pull requests of bugfixes would also be appreciated.

Pinging:

PR contributors:

  • @cedricdonie, https://github.com/sktime/sktime-dl/pull/132 https://github.com/sktime/sktime-dl/pull/127
  • @Vasudeva-bit, https://github.com/sktime/sktime-dl/pull/128
  • @sugam10, https://github.com/sktime/sktime-dl/pull/118
  • @nanashi06, https://github.com/sktime/sktime-dl/pull/115
  • @tabhishek432, https://github.com/sktime/sktime-dl/pull/111

Issue owers:

  • @cedricdonie https://github.com/sktime/sktime/issues/5864
  • @h3dema https://github.com/sktime/sktime-dl/issues/123
  • @loc-trinh https://github.com/sktime/sktime/issues/5819
  • @ozkauysal https://github.com/sktime/sktime/issues/5821
  • @antepenultieme https://github.com/sktime/sktime-dl/issues/119
  • @xunannancy https://github.com/sktime/sktime/issues/5820
  • @arainboldt https://github.com/sktime/sktime/issues/5826

Algorithm maintainers and core contributors: (you have been added to "maintainers" tags)

  • @withington
  • @james-large
  • @jnrusson1
  • @Riyabelle25
  • @ABostrom
  • @nanashi06

fkiraly avatar Jan 24 '24 15:01 fkiraly

@fkiraly could you assign this issue to me as I am going to work on it to ensure that all estimators are migrated

fnhirwa avatar Jul 02 '24 14:07 fnhirwa