JuliaStats
Change signature of `pairwise!` and `colwise!`
This PR fixes https://github.com/JuliaStats/Distances.jl/issues/238 and deprecates pairwise!(r, dist, a[, b]) in favour of pairwise!(dist, r, a[, b]) which is consistent with StatsAPI and StatsBase.
It seemed natural to keep colwise! consistent with pairwise!, and hence I also deprecated colwise!(r, dist, a, b) in favour of colwise!(dist, r, a, b).
Codecov Report
Patch coverage: 100.00% and project coverage change: -4.88 :warning:
Comparison is base (
91f51b5) 97.57% compared to head (dc6f0dd) 92.69%.
Additional details and impacted files
@@ Coverage Diff @@
## master #239 +/- ##
==========================================
- Coverage 97.57% 92.69% -4.88%
==========================================
Files 8 8
Lines 865 520 -345
==========================================
- Hits 844 482 -362
- Misses 21 38 +17
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/Distances.jl | 100.00% <ø> (ø) |
|
| src/generic.jl | 94.05% <100.00%> (-4.68%) |
:arrow_down: |
| src/mahalanobis.jl | 92.50% <100.00%> (-6.24%) |
:arrow_down: |
| src/metrics.jl | 90.81% <100.00%> (-6.10%) |
:arrow_down: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Distances uses the standard convention that the first argument is the one getting mutated. It doesn't feel worth changing this when it has been like this for so long.
Yes, this is definitely a strong argument against changing any of these function signatures. The main motivation for this PR is that pairwise! is not owned by Distances anymore and that the docstring in StatsAPI and the implementation in StatsBase which owns pairwise! use the convention that the second argument is the one being mutated and the first one is the evaluated function. Since the metrics are functors the different function signatures seemed inconsistent and surprising.
It's true that pairwise! has lived in Distances for a long time before it was added to StatsBase and StatsAPI. But it makes sense to me to follow the convention used by map!, broadcast! and get! according to which the evaluated function comes first.
Given that there is no ambiguity, maybe we could allow both forms for some time? This is very similar to deprecating it, except that both would be mentioned in the docs.
Shall we finish this, release as v0.10.x, and subsequently merge #233 and release as v0.11.0? Potentially removing the deprecation warnings right away? Package maintainers will need to bump their Project.toml and would notice errors, whereas they will only get deprectation warnings as long as they don't touch their Project.toml.
Sorry, I missed the last few comments here. I added more tests (it seems currently pairwise! with dims is not tested). I was not sure if the plan is to keep the deprecations in this PR, or if I should remove them as suggested above by @nalimilan.
I would vote for keeping the deprecated.jl file, but remove the deprecations, and add a comment that this should be deprecated in the v0.11 release and removed in the v0.12 release. We could still release v0.11 right after this.
I'd say we can deprecate methods right away, as deprecations are not printed by default anyway so they won't annoy users in normal work. But as you prefer. What I wonder is how we should document these: should we attach a separate docstring to the methods, or add a note mentioning the deprecated argument order in the docstring for standard methods?
Also, how about tagging 0.10.8 with deprecated methods first, and then tag 1.0 so that we can drop deprecated methods in 1.x (e.g. in a few years as there's no hurry)?
@nalimilan I added docstrings to the deprecated methods.
(Regarding releases, I think maybe we might want to merge and release some other outstanding PRs such as #246 first before making a breaking release. I guess it will take a while for a breaking release to be supported throughout the Julia ecosystem and it might be nice to spread goodies such as #246 a bit faster.)
Indeed the same test failures show up in #250 (and hence on the master branch).
The weird thing is that I can't reproduce these test errors locally (Julia 1.9.1, clean environment). So I can't debug them locally.
@nalimilan Good to go anyway?
This API change is appreciated, but it breaks definitions of custom distances. It was a good reason to tag a new minor version.
The old syntax still exists and is only deprecated, so you can still call colwise! and pairwise! with the previous order of arguments. Hence it should not have been breaking. Do you have an example that broke?
I've linked a PR with an example of MySqEuclidean metric that defined pairwise!() using old arguments order.
But the internal functions of Distances.jl were calling it, of course, with the new order.
This seems to be a bug, I should have updated the internal calls to pairwise! and colwise! only in a breaking release and left them unchanged until the deprecations are removed.