Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Initializing any DL estimator with non-default values constructs these values only superficially #4067

Closed
achieveordie opened this issue Jan 5, 2023 · 3 comments · Fixed by #4075
Assignees
Labels
bug Something isn't working module:classification classification module: time series classification module:regression regression module: time series regression

Comments

@achieveordie
Copy link
Collaborator

Describe the bug

A logical bug is present in all DL estimators and is caused by not passing custom parameters to its underlying Network since Network actually makes use of these values to create the DL model of appropriate size. Take a look at the example provided below.

To Reproduce

from pprint import pprint
from classification.deep_learning import CNNClassifier

model = CNNClassifier(
    kernel_size=3,
    n_conv_layers=1,
)

# model.summary() fails due to self.history being None, another bug

pprint(vars(model))
print("=="*25)
pprint(vars(model._network))

Output:

{'_X_metadata': [],
 '_class_dictionary': {},
 '_estimator_type': 'classifier',
 '_is_fitted': False,
 '_network': CNNNetwork(),
 '_tags_dynamic': {},
 '_threads_to_use': 1,
 'activation': 'sigmoid',
 'avg_pool_size': 3,
 'batch_size': 16,
 'callbacks': None,
 'classes_': [],
 'fit_time_': 0,
 'history': None,
 'kernel_size': 3,
 'loss': 'mean_squared_error',
 'metrics': None,
 'model_': None,
 'n_classes_': 0,
 'n_conv_layers': 1,
 'n_epochs': 2000,
 'optimizer': None,
 'random_state': None,
 'use_bias': True,
 'verbose': False}
==================================================
{'_tags_dynamic': {},
 'activation': 'sigmoid',
 'avg_pool_size': 3,
 'filter_sizes': [6, 12],
 'kernel_size': 7,
 'n_conv_layers': 2,
 'random_state': 0}

Expected behavior

This is happening because of self._network = CNNNetwork() (or corresponding Network) in the constructor of the estimator. The values that we receive are to be passed into the Network.

Additional context

I'm speculating this is the reason why the checks at #4036 are failing despite having very small hyperparameters. In any case, I'll open a PR to resolve this bug.

Versions

D:\Anaconda\envs\sktime-dev\lib\site-packages\_distutils_hack\__init__.py:33: UserWarning: Setuptools is replacing distutils.
  warnings.warn("Setuptools is replacing distutils.")

System:
    python: 3.8.13 | packaged by conda-forge | (default, Mar 25 2022, 05:59:00) [MSC v.1929 64 bit (AMD64)]
executable: D:\Anaconda\envs\sktime-dev\python.exe
   machine: Windows-10-10.0.22621-SP0

Python dependencies:
          pip: 22.3.1
   setuptools: 65.5.1
      sklearn: 1.1.3
       sktime: 0.15.0
  statsmodels: 0.13.5
        numpy: 1.22.4
        scipy: 1.9.3
       pandas: 1.5.1
   matplotlib: 3.6.2
       joblib: 1.2.0
        numba: 0.56.4
     pmdarima: 2.0.1
      tsfresh: 0.19.0
@achieveordie achieveordie added the bug Something isn't working label Jan 5, 2023
@achieveordie achieveordie self-assigned this Jan 5, 2023
@achieveordie achieveordie added module:classification classification module: time series classification module:regression regression module: time series regression bugfix Fixes a known bug or removes unintended behavior and removed bugfix Fixes a known bug or removes unintended behavior labels Jan 5, 2023
@fkiraly
Copy link
Collaborator

fkiraly commented Jan 5, 2023

hm, this seems to be pretty systematic in the code base!

A baffling find, as this bug was hiding in plain sight. Kudos for spotting this.

What would be the best fix? Manually changing it all to passthrough of parameters?
Or, moving the neural network object to be a parameter of the estimator?

@achieveordie
Copy link
Collaborator Author

I'm leaning towards manually changing it for all estimators since it minimally changes the current structure/implementation.

Whenever we revise the architecture we can discuss the alternatives. What say?

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 9, 2023

Makes sense!

fkiraly pushed a commit that referenced this issue Jan 11, 2023
…nderlying `Network` object (#4075)

Fixes #4067:

- [x] Manually correct behaviour for all current estimators by passing all values from estimator object to network object during estimator initialization.
- [x] Add a test to ensure this works properly.
klam-data pushed a commit to CodeSmithDSMLProjects/sktime that referenced this issue Jan 18, 2023
…nderlying `Network` object (sktime#4075)

Fixes sktime#4067:

- [x] Manually correct behaviour for all current estimators by passing all values from estimator object to network object during estimator initialization.
- [x] Add a test to ensure this works properly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module:classification classification module: time series classification module:regression regression module: time series regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants