[refactor] Address the Issue#148 and clean _search() function #165
Conversation
Since the readability of the function was quite bad, I separated the processing. I mostly copied and pasted the original processing and checked if we can run some examples.
nabenabe0928
force-pushed
the
refactor_basetask
branch
from
02991d5 to
b7726a8
Compare
| 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.
could you move this comment to line 706?
|
|
||
| return budget_config | ||
|
|
||
| def _start_smac(self, proc_smac: AutoMLSMBO) -> None: |
There was a problem hiding this comment.
I think its fine if we have this function inside _run_smac. Having two functions makes it a bit confusing.
| self._metric: Optional[autoPyTorchMetric] = None | ||
| self._logger: Optional[PicklableClientLogger] = None | ||
| 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.
why do you want to have it as an empty string?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.