Skip to content

Conversation

ngupta23
Copy link
Collaborator

Describe the changes you've made

Added more scikit models

  • dt_cds_dt
  • ridge_cds_dt
  • lasso_cds_dt
  • lar_cds_dt
  • llar_cds_dt
  • br_cds_dt
  • lr_cds_dt
  • huber_cds_dt
  • par_cds_dt
  • omp_cds_dt
  • en_cds_dt
  • gbr_cds_dt
  • ada_cds_dt
  • knn_cds_dt

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Unit tests are passing locally

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

None

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

@ngupta23 ngupta23 requested review from Yard1 and TremaMiguel and removed request for Yard1 and TremaMiguel April 25, 2021 23:53
@ngupta23 ngupta23 marked this pull request as draft April 25, 2021 23:55
@ngupta23 ngupta23 requested review from Yard1 and TremaMiguel April 26, 2021 00:37
@ngupta23 ngupta23 marked this pull request as ready for review April 26, 2021 00:37
@ngupta23 ngupta23 marked this pull request as draft April 26, 2021 00:37
@ngupta23
Copy link
Collaborator Author

Sorry, don't review for now. I will merge with @TremaMiguel 's eventual PR and then you can review it.

@ngupta23 ngupta23 marked this pull request as ready for review May 1, 2021 11:32
@ngupta23 ngupta23 requested review from Yard1 and TremaMiguel May 1, 2021 11:32
#### CLASSICAL STATISTICAL MODELS ####
######################################


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make more clear and clean the __init__ of the ArimaContainer, is it possible to decompose this process into methods that get called in the __init__ ?. For example return_order_related_params can be a method of this class called during init

class ArimaContainer(TimeSeriesContainer):

    def __init__():
          ....
         self.return_order_related_params()

    def return_order_related_params(self):

"regressor__fit_intercept": [True, False],
"regressor__normalize": [True, False],
}
tune_distributions = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, it is not easy to follow the __init__ method, maybe for all classes could go methods like get_tune_grid and get_tune_distributions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe these methods get_tune_grid and get_tune_distributions can be a part of the parent class TimeSeriesExperiment, we can actually force each container to have this method trough an abstractmethod.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea. Currently, I was just following the approach that already existed in other modules, but this modularization is definitely something that I would like to have as well. Since it can potentially be leveraged by other tasks as well (regression, classification, etc.), lets quickly discuss with @Yard1 before making this change. I will open a Issue for this so we keep track of it,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks Nikhil.

@ngupta23 ngupta23 merged commit e513820 into time_series May 2, 2021
@ngupta23 ngupta23 deleted the ts_scikit_models branch May 2, 2021 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants