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

Initial additions for Random Grid Search for Time Series #1164

Merged
merged 4 commits into from Apr 16, 2021

Conversation

ngupta23
Copy link
Collaborator

Describe the changes you've made

Added initial code for Random Grid Search for TS

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Unit Tests have been added

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

NA

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 TremaMiguel and Yard1 and removed request for TremaMiguel April 14, 2021 12:22
self.n_iter = n_iter
self.random_state = random_state

def _run_search(self, evaluate_candidates):
Copy link
Contributor

Choose a reason for hiding this comment

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

the evaluate_candidates is callable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. its defined in the GridSearch class.

Copy link
Contributor

Choose a reason for hiding this comment

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

even though it is a "private" method, perhaps we could raise an error if evaluate_candidates is not callable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we are controlling that, it should be ok. In any case, it is mimicking the sklearn and sktime implementation so we should be ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@TremaMiguel TremaMiguel left a comment

Choose a reason for hiding this comment

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

Looks good, just added a pair of comments.

Copy link
Member

@Yard1 Yard1 left a comment

Choose a reason for hiding this comment

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

LGTM, just one thing here - in general distributions should be left for external search libraries, for normal random/grid search just use tune_grid with lists

@@ -240,7 +245,10 @@ def __init__(self, globals_dict: dict) -> None:
"degree": [1,2,3,4,5],
"with_intercept": [True, False]
}
tune_distributions = {}
tune_distributions = {
"degree": randint(low=1, high=10),
Copy link
Member

Choose a reason for hiding this comment

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

It would be best if you used PyCaret's distributions (pycaret/internal/distributions.py) - feel free to expand upon them if necessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @Yard1 , Any particular reason why? I looked at that before implementing this, but it looks like that is needed only because you are interfacing with many external GS libraries and methods. In this case, it is all internal since it needs custom argument ordering (y, X vs X, y), etc so it wont work with something like BayesGridSearchCV from skoptimize unless we implement out own version custom for TS.

Do you see any issues leaving it as is?

Copy link
Member

Choose a reason for hiding this comment

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

I would want to keep it consistent so that it will be easier when we implement wrappers for search algos in the future. Distributions would be the same, after all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Yard1 , I have implemented the changes to use PyCaret distributions. Please have a look

@ngupta23 ngupta23 merged commit 66b0f1c into time_series Apr 16, 2021
@ngupta23 ngupta23 deleted the ts_random2 branch April 16, 2021 09:20
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.

None yet

3 participants