Extend output result & minor fixes #81
Conversation
|
Pep8 is also important. There's an extra space somewhere :) |
bench.py
Outdated
| return str(data.dtypes[0]) | ||
| elif hasattr(data, 'values'): | ||
| if hasattr(data, 'values'): | ||
| return data.values.dtype | ||
| else: |
There was a problem hiding this comment.
just raise an exception here, without else
bench.py
Outdated
| else: | ||
| return optimal_cache_size_bytes | ||
| return max_cache_bytes if optimal_cache_size_bytes > max_cache_bytes \ | ||
| else optimal_cache_size_bytes |
There was a problem hiding this comment.
An extra comma at the end of line 175 'Now avalible for scikit-learn benchmarks'),
bench.py
Outdated
| y = convert_to_numpy(y) | ||
| yp = convert_to_numpy(yp) | ||
| if y.ndim + yp.ndim > 2: |
There was a problem hiding this comment.
is that code really useless? does sklearn_accuracy work same?
bench.py
Outdated
| try: | ||
| res = sklearn_log_loss(y, yp) | ||
| except Exception: | ||
| res = None |
There was a problem hiding this comment.
Handling exceptions like this is a bad practice: every mistake will be hidden by this code
Is log_loss always properly called?
Is it always given probabilities and not labels?
Is the argument order right?
Same questions should be answered for other metrics
sklearn_bench/dbscan.py
Outdated
| acc = davies_bouldin_score(X, labels) | ||
| try: | ||
| acc = davies_bouldin_score(X, labels) | ||
| except Exception: |
There was a problem hiding this comment.
It would be better to have all scoring functions in the same place (bench.py)
And again, such handling is a bad practice
sklearn_bench/dbscan.py
Outdated
| data=[X], alg_instance=dbscan) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| parser = argparse.ArgumentParser(description='scikit-learn DBSCAN benchmark') | ||
| parser.add_argument('-e', '--eps', '--epsilon', type=float, default=10., | ||
| parser.add_argument('-e', '--eps', '--epsilon', type=float, default=0.5, |
There was a problem hiding this comment.
why did you change it? this will not affect the measurements of the current configs?
There was a problem hiding this comment.
It is default value in sklearn
There was a problem hiding this comment.
this will not affect the measurements of the current configs?
bench.py
Outdated
| try: | ||
| res = sklearn_dbs(y_true, y_pred) | ||
| except ValueError: | ||
| res = "Number of labels is 1" |
There was a problem hiding this comment.
for example: y _pred= [1, 1, 1, 1, 1, 1, 1,] ? Or?
sklearn_bench/dbscan.py
Outdated
| data=[X], alg_instance=dbscan) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| parser = argparse.ArgumentParser(description='scikit-learn DBSCAN benchmark') | ||
| parser.add_argument('-e', '--eps', '--epsilon', type=float, default=10., | ||
| parser.add_argument('-e', '--eps', '--epsilon', type=float, default=0.5, |
There was a problem hiding this comment.
this will not affect the measurements of the current configs?
| res = sklearn_dbs(X, labels) | ||
| except ValueError as ex: | ||
| res = ex | ||
| return res |
There was a problem hiding this comment.
I'm slightly concerned about such exception handling. My opinion is - our own configs shouldn't cause any exceptions.
And I would have rather removed every handling from the code
This reverts commit 972efac.
No description provided.