- Notifications
You must be signed in to change notification settings - Fork302
[refactor] Address the Issue#148 and clean _search() function#165
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Since the readability of the function was quite bad,I separated the processing.I mostly copied and pasted the original processing andchecked if we can run some examples.
02991d5 tob7726a8Compare| self._stopwatch.stop_task(dummy_task_name) | ||
| def_run_traditional_ml(self)->None: | ||
| """We would like to obtain training time for at least 1 Neural network in SMAC""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could you move this comment to line 706?
| returnbudget_config | ||
| def_start_smac(self,proc_smac:AutoMLSMBO)->None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think its fine if we have this function inside _run_smac. Having two functions makes it a bit confusing.
| self.run_history:RunHistory=RunHistory() | ||
| self.trajectory:Optional[List]=None | ||
| self.dataset_name:Optional[str]=None | ||
| self.dataset_name:str="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why do you want to have it as an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay, I see that it was suggested by Francisco in a previous PR. I suggest you to not make the same changes in different PR. Let merge#106 first and then we can rebase to include this change. It makes it confusing to review the same changes in different places.
ravinkohli left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey, Thanks for the PR. While I agree with dividing _search into smaller functions. Do you think too many functions have been created now? I feel if you could merge the functions that are doing one thing for example _do_traditional_predictions can be merged into _run_traditional_ml and same for dummy, and smac it would make it much easier to understand whats going on in the code. And while you are making these functions, it would be great to also add documentation for each one. as well as keeping the comments that were there before each process for example=====> Run dummy, etc. Also, I think its better if we keep the _search_settings a part of the search function.
For more details, see#148.