Change signature of `pairwise!` and `colwise!` #239

devmotion · 2022-02-01T20:20:45Z

This PR fixes #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-commenter · 2022-02-01T20:27:41Z

Codecov Report

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

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%)`	⬇️
src/mahalanobis.jl	`92.50% <100.00%> (-6.24%)`	⬇️
src/metrics.jl	`90.81% <100.00%> (-6.10%)`	⬇️

... and 4 files with indirect coverage changes

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

KristofferC · 2022-02-01T20:52:16Z

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.

devmotion · 2022-02-01T22:57:42Z

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.

nalimilan · 2022-03-12T22:25:05Z

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.

nalimilan

Sorry I completely missed this! Looks good, indeed the new order is consistent with map!. Just a small remark about a missing test, given that it's essential existing code continues to work for a long time all arguments should be tested.

nalimilan · 2022-06-10T19:55:22Z

test/test_dists.jl

+            rxy2 = similar(rxy)
+            @test @test_deprecated(pairwise!(rxy2, dist, vecx, vecy)) ≈ rxy
+            @test rxy2 ≈ rxy
+            rxy3 = similar(rxy)
+            @test pairwise!(dist, rxy3, vecx, vecy) ≈ rxy
+            @test rxy3 ≈ rxy
+
+            rxx2 = similar(rxx)
+            @test @test_deprecated(pairwise!(rxx2, dist, vecx)) ≈ rxx
+            @test rxx2 ≈ rxx
+            rxx3 = similar(rxx)
+            @test pairwise!(dist, rxx3, vecx) ≈ rxx
+            @test rxx3 ≈ rxx


Also test passing dims?

These tests here are only for general arguments such as iterators or vectors of vectors. There are actually no tests of pairwise! with dims currently. I added them in c43e9dc.

dkarrasch · 2022-09-12T15:20:06Z

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.

devmotion · 2022-09-12T18:36:14Z

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.

dkarrasch · 2022-09-13T08:04:13Z

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.

nalimilan · 2022-09-13T12:28:17Z

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)?

devmotion · 2023-06-30T13:03:13Z

@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.)

nalimilan

Looks good, but I'm a bit worried about the failing tests. They seem unrelated, but I couldn't find a way to run CI on master to confirm that.

devmotion · 2023-07-01T10:08:45Z

I triggered CI in #250.

devmotion · 2023-07-01T11:03:25Z

Indeed the same test failures show up in #250 (and hence on the master branch).

devmotion · 2023-07-03T12:08:38Z

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?

alyst · 2023-07-26T08:09:28Z

This API change is appreciated, but it breaks definitions of custom distances.
It was a good reason to tag a new minor version.

devmotion · 2023-07-26T08:25:57Z

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?

alyst · 2023-07-26T08:35:17Z

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.

devmotion · 2023-07-26T09:30:13Z

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.

devmotion added 3 commits February 1, 2022 20:54

Fix signature of pairwise!

ee71d01

Consistent signature of colwise!

ae485e7

Bump version

8a6bff2

nalimilan reviewed Jun 10, 2022

View reviewed changes

Add more tests

c43e9dc

Add docstrings

dc6f0dd

nalimilan approved these changes Jul 1, 2023

View reviewed changes

devmotion mentioned this pull request Jul 1, 2023

DO NOT MERGE - Trigger CI #250

Closed

nalimilan merged commit d05bf6c into master Jul 3, 2023

nalimilan deleted the dw/pairwise! branch July 3, 2023 13:48

alyst mentioned this pull request Jul 26, 2023

Update to Distances.jl 0.10.9 JuliaStats/Clustering.jl#259

Merged

devmotion mentioned this pull request Oct 6, 2023

Stop using deprecated signatures from Distances.jl JuliaGaussianProcesses/KernelFunctions.jl#529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change signature of `pairwise!` and `colwise!` #239

Change signature of `pairwise!` and `colwise!` #239

devmotion commented Feb 1, 2022

codecov-commenter commented Feb 1, 2022 •

edited

Loading

KristofferC commented Feb 1, 2022 •

edited

Loading

devmotion commented Feb 1, 2022

nalimilan commented Mar 12, 2022

nalimilan left a comment

nalimilan Jun 10, 2022

devmotion Sep 12, 2022 •

edited

Loading

dkarrasch commented Sep 12, 2022

devmotion commented Sep 12, 2022

dkarrasch commented Sep 13, 2022

nalimilan commented Sep 13, 2022

devmotion commented Jun 30, 2023

nalimilan left a comment

devmotion commented Jul 1, 2023

devmotion commented Jul 1, 2023

devmotion commented Jul 3, 2023

alyst commented Jul 26, 2023

devmotion commented Jul 26, 2023

alyst commented Jul 26, 2023

devmotion commented Jul 26, 2023

Change signature of pairwise! and colwise! #239

Change signature of pairwise! and colwise! #239

Conversation

devmotion commented Feb 1, 2022

codecov-commenter commented Feb 1, 2022 • edited Loading

Codecov Report

KristofferC commented Feb 1, 2022 • edited Loading

devmotion commented Feb 1, 2022

nalimilan commented Mar 12, 2022

nalimilan left a comment

Choose a reason for hiding this comment

nalimilan Jun 10, 2022

Choose a reason for hiding this comment

devmotion Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

dkarrasch commented Sep 12, 2022

devmotion commented Sep 12, 2022

dkarrasch commented Sep 13, 2022

nalimilan commented Sep 13, 2022

devmotion commented Jun 30, 2023

nalimilan left a comment

Choose a reason for hiding this comment

devmotion commented Jul 1, 2023

devmotion commented Jul 1, 2023

devmotion commented Jul 3, 2023

alyst commented Jul 26, 2023

devmotion commented Jul 26, 2023

alyst commented Jul 26, 2023

devmotion commented Jul 26, 2023

Change signature of `pairwise!` and `colwise!` #239

Change signature of `pairwise!` and `colwise!` #239

codecov-commenter commented Feb 1, 2022 •

edited

Loading

KristofferC commented Feb 1, 2022 •

edited

Loading

devmotion Sep 12, 2022 •

edited

Loading