Closed giovannidoni closed 4 years ago
Thanks for adding a check on the metric for the case it is callable, and the corresponding test function.
Among the cosmetic changes I cannot accept those removing self.k_max_
and self.dim_
: the attributes self.k_max
and self.dim
should not be modified, and thus the new public attributes self.k_max_
and self.dim_
are defined in the fit
function.
Indeed the test_common.py
test fails with the following errors:
E AssertionError: Estimator DensityPeakAdvanced should not change or mutate the parameter dim from None to 2 during fit.
E AssertionError: Estimator changes public attribute(s) during the fit method. Estimators are only allowed to change attributes started or ended with _, but dim changed
=============================================== short test summary info ================================================
FAILED test_common.py::test_scikitlearn_compatible_estimator[DensityPeakAdvanced()-check_estimators_overwrite_params]
FAILED test_common.py::test_scikitlearn_compatible_estimator[DensityPeakAdvanced()-check_dont_overwrite_parameters]
============================================= 2 failed, 87 passed in 1.07s =============================================
It would also be better to have separate commits for the callable metric and the cosmetic changes.
See pr #2
A callable should be a viable distance metric to be used as per the docstring, however the constructor raises an error if a callable is passed. Added a few costmetic changes.