Refactor dbscan()

[alyst]

We have two implementations of dbscan(): one is distance matrix-based, another is NNTree-based. As noted in #221, they have inconsistent APIs and return different type.
This PR consolidates the two implementations (based on NNTree one), fixes some behaviour differences and sets deprecations:

• dbscan(dist, eps, minpts) is deprecated in favor of dbscan(dist, radius, metric=nothihg, min_neighbors=minpts)
• dbscan() adds return_type= parameter, which is Vector{DbscanCluster} for dbscan(points, ...), but this behaviour is deprecated in favor of return_type=DbscanResult.
  dbscan() now always returns DbscanResult. This is a breaking change for NN-tree-based implementation, which returned Vector{DbscanCluster}.
• fixes a bug in NNTree-based implementation -- the check that the cluster core is not empty was missing. See e.g. DBscan Adjacency Lists have repeated clusters #200
• radius=eps=0 is allowed for both implementations
• min_neighbors now includes the point itself (as in the original pseudocode and distance matrix-based impl)
• metric allows specifying non-Euclidean metrics; when metric=nothing, it signifies that the matrix provided is the distance matrix.

[alyst]

@ararslan Sorry for bothering again. I've tried to fix the inconsistencies of the two dbscan() implementations we have: they differ in return type and positional arguments. It required creating two deprecations to maintain backward compatibility.
But potentially there are other alternatives for the consolidated API and deprecation path. Let me know what you think.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.11 ⚠️

Comparison is base (04b0705) 95.22% compared to head (7782248) 95.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #248      +/-   ##
==========================================
- Coverage   95.22%   95.11%   -0.11%     
==========================================
  Files          18       18              
  Lines        1339     1310      -29     
==========================================
- Hits         1275     1246      -29     
  Misses         64       64              

Impacted Files	Coverage Δ
src/deprecate.jl	44.44% <ø> (ø)
src/dbscan.jl	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ararslan]

Sorry for bothering again

No worries! Apologies for the delay, I've been insanely busy lately and have fallen quite far behind on GitHub notifications...

It's been so long since I last looked at this package that I don't feel like I'm well suited to judge correctness. I do wonder whether an easier deprecation path would be one of these:

• Deprecate the method that doesn't return a DbscanResult in favor of simply wrapping the call in DbscanResult, i.e.

  @deprecate dbscan(...) DbscanResult(dbscan(...))

• Just change the return type of the method that doesn't return a DbscanResult without a deprecation and tag a breaking release. A description of user-facing changes can be added to the GitHub release so people can determine how to be compatible with the new version.

I feel like the latter might actually be easiest for both users and maintainers. Thoughts?

[alyst]

I feel like the latter might actually be easiest for both users and maintainers. Thoughts?

Thanks for allowing me the breaking change :)
It is a much more straightforward path + NN-tree based dbscan() (the one that returned Vector{DbscanCluster}) gave incorrect results anyway,
so the breaking change will alert the users that they should expect very different clustering.