-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix#317 #484
Fix#317 #484
Conversation
Fix minor printing error in sprint_statistics.
Revert "Fix#460"
…into development
Codecov Report
@@ Coverage Diff @@
## development #484 +/- ##
===============================================
- Coverage 78.63% 78.56% -0.08%
===============================================
Files 130 130
Lines 10098 10110 +12
===============================================
+ Hits 7941 7943 +2
- Misses 2157 2167 +10
Continue to review full report at Codecov.
|
autosklearn/util/backend.py
Outdated
# Check that the names of tmp_dir and output_dir is not the same. | ||
if temporary_directory == output_directory \ | ||
and temporary_directory is not None: | ||
raise ValueError("The names of tmp dir and output dir " |
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 please change this to The temporary and the output directory must be different
?
autosklearn/util/backend.py
Outdated
# attributes to check that directories were created by autosklearn. | ||
self._tmp_dir_created = False | ||
self._output_dir_created = False | ||
self._prepare_directories(temporary_directory, output_directory) |
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 please add a line break between the variables and the function calls to make it visually easier to see the difference?
autosklearn/util/backend.py
Outdated
self._logger = logging.get_logger(__name__) | ||
self.create_directories() | ||
|
||
@property | ||
def output_directory(self): | ||
return self.__output_directory | ||
# make sure that tilde does not appear on the path. | ||
return os.path.expanduser(self.__output_directory) |
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.
Please add os.path.expandvars
.
autosklearn/util/backend.py
Outdated
|
||
@property | ||
def temporary_directory(self): | ||
return self.__temporary_directory | ||
# make sure that tilde does not appear on the path. | ||
return os.path.expanduser(self.__temporary_directory) |
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.
and here as well.
autosklearn/util/backend.py
Outdated
|
||
def __del__(self): | ||
self.delete_directories(force=False) | ||
|
||
def delete_directories(self, force=True): | ||
if self.delete_output_folder_after_terminate or force: | ||
if self._output_dir_created is False and self.shared_mode is False: | ||
raise OSError("Failed to delete output dir: %s " |
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.
Please do not raise OSError
here as this is not raised by the operating system. ValueError
should be fine.
autosklearn/util/backend.py
Outdated
@@ -86,6 +121,12 @@ def delete_directories(self, force=True): | |||
self.output_directory) | |||
|
|||
if self.delete_tmp_folder_after_terminate or force: | |||
if self._tmp_dir_created is False and self.shared_mode is False: | |||
raise OSError("Failed to delete tmp dir: % s " |
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.
Same here.
test/test_automl/base.py
Outdated
os.makedirs(output) | ||
except OSError: | ||
pass | ||
# This part is no longer necessary as the directory creating |
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.
Please delete these lines.
test/test_automl/test_automl.py
Outdated
@@ -43,8 +43,11 @@ def setUp(self): | |||
self.automl._delete_output_directories = lambda: 0 | |||
|
|||
def test_refit_shuffle_on_fail(self): | |||
output = os.path.join(self.test_dir, '..', '.tmp_refit_shuffle_on_fail') | |||
context = BackendContext(output, output, False, False) | |||
tmp = os.path.join(self.test_dir, '..', '.tmp._refit_shuffle_on_fail') |
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 please create a helper function for these lines, which returns a backend object?
test/test_automl/test_automl.py
Outdated
@@ -110,6 +116,7 @@ def test_fit(self): | |||
self.assertEqual(automl._task, MULTICLASS_CLASSIFICATION) | |||
|
|||
del automl | |||
self._tearDown(tmp) |
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.
and also a teardown helper function which accepts the backend as input?
Fixes #317.