-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Qualify pairwise call #360
base: master
Are you sure you want to change the base?
Conversation
Interesting. I'm intruiged to see precisely which lines have changed coverage in this PR (which I guess we'll see once the Format suggestions check passes?). If there is indeed no need for certain methods, I agree that it would make sense to remove them. With things like |
I have covered a few more lines, and run the formatter, so that should pass now, I hope. I feel the solution could be to keep the tests in place, remove the qualifiers, and then also remove all non-covered lines. The test will then make sure that the functionality stays. |
After some digging around in Distances, I discovered this PR JuliaStats/Distances.jl#194 , that I had previously missed. It resolves the long-standing issue that we had whereby we couldn't add some methods that we needed to What concerns me now is just AD-related stuff. Specifically, This brings me around to what I think should be done:
My suspicion is that the majority of the methods that we have will be necessary for AD to continue to work, and we're not currently testing that at all properly. The "correct" way to do that in the modern world is using https://juliadiff.org/ChainRulesTestUtils.jl/dev/#Testing-AD-systems . This leaves a couple of ways forward:
I'm happy to go with either, it's just a question of what you've got the time / inclination to do. |
Very interesting. I have seen the issue on conditional dependencies, it does not seem that it will resolve very soon. In principle I am open to add the proper tests, as a way to learn about AD. I have been using it, but don't really understand it in any way. In practice, I have no idea how difficult it will be. |
Indeed -- to be honest, it would make sense to test all of the methods (once you've got a function to test one of them, testing the others should be straightforward, at least in principle). I think the way to go about it is just going to be to use the method I linked above (definitely don't try to roll your own testing code for AD). I suspect you can just use the rule config defined here: https://github.com/FluxML/Zygote.jl/blob/78bb9a3cad52de6e7c9a590d0f8ac4b6014a73f4/src/compiler/chainrules.jl#L4 |
I noticed that some recent changes in Distances make it necessary to analyze more carefully which methods are called. Distances.pairwise is not owned by Distances anymore. Instead it is owned by StatsBase (even though technically it is defined in StatsAPI). The dangerous part is that StatsBase defines a very general fallback definition of pairwise that does not exploit the structure of the metrics and inputs. However, it is difficult to spot potential performance problems since (as intended) no errors or warnings are shown if the potentially slow fallback is hit. It even caused problems with Distances.PreMetric, for some inputs the fallback was used instead of the implementations in Distances (I don't remember all details but I added some fixes to Distances a while ago). So while tests might not fail if specific methods are removed we should carefully evaluate if the removal causes any performance regressions. |
After some exploration, I notice two issues:
|
Oh interesting. I have no idea what to do about that. Would suggest using 1.6 if it's not obvious how to fix.
Hmmm could you provide a MWE? |
I think I have the same problem that makes the CI fail all the way through for Julia nightly. I have switched to 1.3 for now. For development, I have started by looking at the following: A = [rand(3) for _ in 1:5]
B = [rand(3) for _ in 1:7]
norm(pairwise(SqEuclidean(), A, B))
ReverseDiff.gradient(a->norm(pairwise(SqEuclidean(), a, B)), A) which yields ERROR: LoadError: MethodError: no method matching zero(::Type{Array{Float64,1}}) With my limited understanding, I have been able to find this discourse discussion, but before going down that road, I want to make sure this is even what I should be looking at. |
Sorry for the slow response @Crown421 I would suggest starting with Zygote, and In particular, if you take a look at the docs on testing AD systems, and utilise the method implemented in Zygote and Zygote's config. I personally generally find this to be the most debuggable bit of testing infrastructure. I'm imagining something along the lines of f(A, B) = pairwise(SqEuclidean(), A, B)
test_rrule(Zygote.ZygoteConfig(), f, A, B; rrule_f=rrule_via_ad) |
While trying to understand how to work with Coverage I was struck by some lines that stubbornly did not get covered. Initially I thought that was due to missing tests, but even adding those did not change the coverage.
After some effort, I have found that these functions don't get called because the default
Distances.pairwise
is already enough. I managed to get those lines covered by qualifying thepairwise
call in the tests, but it seems they are not actually necessary? Maybe it makes sense to remove those definitions from src altogether?