Skip to content
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

fix Distances compat #423

Merged
merged 10 commits into from
Jan 13, 2022
Merged

fix Distances compat #423

merged 10 commits into from
Jan 13, 2022

Conversation

st--
Copy link
Member

@st-- st-- commented Jan 11, 2022

Aims to resolve #390 and #391

@@ -21,7 +21,13 @@
compare_gradient(:Zygote, [x, y]) do xy
KernelFunctions.Sinus(r)(xy[1], xy[2])
end
compare_gradient(:Zygote, [Q, x, y]) do xy
SqMahalanobis(xy[1])(xy[2], xy[3])
if VERSION < v"1.6"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests seemed to pass fine as is on 1.6, and I don't want to skip checks if I can avoid it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but how is the Julia version relevant here? It does not affect the version of Distances that is used, so why can't we use the same test for all Julia versions? It also seems tests fail on Julia 1.3 currently.

Copy link
Member Author

@st-- st-- Jan 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant because it only fails in 1.3 (and 1.4). It passes with 1.6 and 1.7. [not tested on 1.5]

Should we just skip the test for 1.3? Or raise julia compat to 1.4 (part of Ubuntu 20.04) or 1.6 (LTS)?

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #423 (ae34ff2) into master (93d33c2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #423   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files          52       52           
  Lines        1199     1199           
=======================================
  Hits         1112     1112           
  Misses         87       87           
Impacted Files Coverage Δ
src/chainrules.jl 87.65% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93d33c2...ae34ff2. Read the comment docs.

@st-- st-- mentioned this pull request Jan 13, 2022
15 tasks
@st-- st-- requested a review from theogf January 13, 2022 16:35
@st-- st-- merged commit d1c68a9 into master Jan 13, 2022
@st-- st-- deleted the st/fix_distances branch January 13, 2022 21:33
Crown421 added a commit that referenced this pull request Jan 28, 2022
commit f9bbd84
Author: st-- <[email protected]>
Date:   Fri Jan 28 09:11:50 2022 +0100

    make nystrom work with AbstractVector (#427)

    * make nystrom work with AbstractVector

    * add test

    * Update test/approximations/nystrom.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * patch bump

    * Update test/approximations/nystrom.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * Apply suggestions from code review

    Co-authored-by: David Widmann <[email protected]>

    * Apply suggestions from code review

    * deprecate

    * Apply suggestions from code review

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * Apply suggestions from code review

    Co-authored-by: David Widmann <[email protected]>

    * Apply suggestions from code review

    Co-authored-by: Théo Galy-Fajou <[email protected]>

    * Update src/approximations/nystrom.jl

    Co-authored-by: Théo Galy-Fajou <[email protected]>

    * Update src/approximations/nystrom.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: David Widmann <[email protected]>
    Co-authored-by: Théo Galy-Fajou <[email protected]>

commit d1c68a9
Author: st-- <[email protected]>
Date:   Thu Jan 13 22:33:43 2022 +0100

    fix Distances compat (#423)

    * CompatHelper: bump compat for Distances to 0.10 for package test, (keep existing compat)

    * try out Theo's fix

    * fix test compat

    * use ForwardDiff for chain rule test of SqMahalanobis

    * test on 1.4 instead of 1.3 - see if the chainrules test passes there

    * revert version branch

    * revert to 1.3

    * test_broken for older Julia versions

    Co-authored-by: CompatHelper Julia <[email protected]>

commit 93d33c2
Author: st-- <[email protected]>
Date:   Wed Jan 12 14:11:14 2022 +0100

    fix figure & cleanup (#422)

    * fix figure & cleanup

    * bump LIBSVM compat & Manifest

    * improve writing, replaces #321

commit 40cb59e
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Wed Jan 12 09:39:01 2022 +0200

    CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) (#367)

    * CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat)

    * ] up

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit 7204529
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Tue Jan 11 18:37:23 2022 +0200

    CompatHelper: bump compat for Kronecker to 0.5 for package test, (keep existing compat) (#366)

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit 924925d
Author: st-- <[email protected]>
Date:   Tue Jan 11 16:26:02 2022 +0100

    switch SVM example to half-moon dataset (#421)

commit 992b665
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Fri Dec 24 12:18:56 2021 +0200

    CompatHelper: bump compat for SpecialFunctions to 2 for package test, (keep existing compat) (#412)

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: Théo Galy-Fajou <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit 04fa7f7
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Thu Dec 23 13:33:59 2021 +0200

    CompatHelper: bump compat for SpecialFunctions to 2, (keep existing compat) (#411)

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: Théo Galy-Fajou <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit c0fc3e1
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Thu Dec 23 01:10:40 2021 +0200

    CompatHelper: add new compat entry for Compat at version 3 for package test, (keep existing compat) (#418)

    Co-authored-by: CompatHelper Julia <[email protected]>

commit 05fe340
Author: st-- <[email protected]>
Date:   Tue Dec 21 00:49:37 2021 +0200

    use only() instead of first() (#403)

    * use only() instead of first() for 1-"vectors" that were for the benefit of Flux

    * fix one test that should not have worked as it was

    * add missing scalar Sinus constructor

commit 2d17212
Author: st-- <[email protected]>
Date:   Sat Dec 18 23:43:30 2021 +0200

    Zygote AD failure workarounds & test cleanup (#414)

    Zygote AD failures:

    * revert #409 (test_utils workaround for broken Zygote - now working again)

    * disable broken Zygote AD test for ChainTransform

    Improved tests:

    * finer-grained testsets

    * add missing test cases to test_AD

    * replace test_FiniteDiff with test_AD(..., :FiniteDiff, ...)

    * remove code duplication

commit 3c49949
Author: Théo Galy-Fajou <[email protected]>
Date:   Wed Nov 24 18:32:19 2021 +0100

    Fix typo in valid_inputs error (#408)

    * Fix typo in valid_inputs error

    * Update src/utils.jl

    Co-authored-by: David Widmann <[email protected]>

    Co-authored-by: David Widmann <[email protected]>

commit 9955044
Author: st-- <[email protected]>
Date:   Wed Nov 24 18:55:18 2021 +0200

    Fix for Zygote 0.6.30 breaking our tests (#409)

    * restrict Zygote to <0.6.30

    * revert Zygote test restriction and add finer-grained testset

    * Update test/utils.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * revert testset

    * mark test_broken

    * Use `@test_throws` instead of `@test_broken`

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: David Widmann <[email protected]>

commit 33d64d1
Author: Théo Galy-Fajou <[email protected]>
Date:   Thu Nov 4 14:23:57 2021 +0100

    Add benchmarking CI (#399)

    * Add benchmark file

    * delete old benchmarks

    * Add github action

    * Add Project

    * Apply suggestions from code review

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * Missing end of line

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

commit 360ce10
Author: David Widmann <[email protected]>
Date:   Tue Nov 2 11:09:58 2021 +0100

    Update docstring of `GibbsKernel` (#395)
st-- added a commit that referenced this pull request Mar 21, 2022
* initial version of training kernel parameters example from st/examples (#234)

* update script

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix out-of-domain initial value

* initial version of training kernel parameters example from st/examples (#234)

* update script

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* fix out-of-domain initial value

* Add ParameterHandling

* Extend example

* Failed attempts with default Flux opt

* Add Flux.destructure example

* Add some nothing

* Squashed commit of the following:

commit f9bbd84
Author: st-- <[email protected]>
Date:   Fri Jan 28 09:11:50 2022 +0100

    make nystrom work with AbstractVector (#427)

    * make nystrom work with AbstractVector

    * add test

    * Update test/approximations/nystrom.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * patch bump

    * Update test/approximations/nystrom.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * Apply suggestions from code review

    Co-authored-by: David Widmann <[email protected]>

    * Apply suggestions from code review

    * deprecate

    * Apply suggestions from code review

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * Apply suggestions from code review

    Co-authored-by: David Widmann <[email protected]>

    * Apply suggestions from code review

    Co-authored-by: Théo Galy-Fajou <[email protected]>

    * Update src/approximations/nystrom.jl

    Co-authored-by: Théo Galy-Fajou <[email protected]>

    * Update src/approximations/nystrom.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: David Widmann <[email protected]>
    Co-authored-by: Théo Galy-Fajou <[email protected]>

commit d1c68a9
Author: st-- <[email protected]>
Date:   Thu Jan 13 22:33:43 2022 +0100

    fix Distances compat (#423)

    * CompatHelper: bump compat for Distances to 0.10 for package test, (keep existing compat)

    * try out Theo's fix

    * fix test compat

    * use ForwardDiff for chain rule test of SqMahalanobis

    * test on 1.4 instead of 1.3 - see if the chainrules test passes there

    * revert version branch

    * revert to 1.3

    * test_broken for older Julia versions

    Co-authored-by: CompatHelper Julia <[email protected]>

commit 93d33c2
Author: st-- <[email protected]>
Date:   Wed Jan 12 14:11:14 2022 +0100

    fix figure & cleanup (#422)

    * fix figure & cleanup

    * bump LIBSVM compat & Manifest

    * improve writing, replaces #321

commit 40cb59e
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Wed Jan 12 09:39:01 2022 +0200

    CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) (#367)

    * CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat)

    * ] up

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit 7204529
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Tue Jan 11 18:37:23 2022 +0200

    CompatHelper: bump compat for Kronecker to 0.5 for package test, (keep existing compat) (#366)

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit 924925d
Author: st-- <[email protected]>
Date:   Tue Jan 11 16:26:02 2022 +0100

    switch SVM example to half-moon dataset (#421)

commit 992b665
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Fri Dec 24 12:18:56 2021 +0200

    CompatHelper: bump compat for SpecialFunctions to 2 for package test, (keep existing compat) (#412)

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: Théo Galy-Fajou <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit 04fa7f7
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Thu Dec 23 13:33:59 2021 +0200

    CompatHelper: bump compat for SpecialFunctions to 2, (keep existing compat) (#411)

    Co-authored-by: CompatHelper Julia <[email protected]>
    Co-authored-by: Théo Galy-Fajou <[email protected]>
    Co-authored-by: st-- <[email protected]>

commit c0fc3e1
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Thu Dec 23 01:10:40 2021 +0200

    CompatHelper: add new compat entry for Compat at version 3 for package test, (keep existing compat) (#418)

    Co-authored-by: CompatHelper Julia <[email protected]>

commit 05fe340
Author: st-- <[email protected]>
Date:   Tue Dec 21 00:49:37 2021 +0200

    use only() instead of first() (#403)

    * use only() instead of first() for 1-"vectors" that were for the benefit of Flux

    * fix one test that should not have worked as it was

    * add missing scalar Sinus constructor

commit 2d17212
Author: st-- <[email protected]>
Date:   Sat Dec 18 23:43:30 2021 +0200

    Zygote AD failure workarounds & test cleanup (#414)

    Zygote AD failures:

    * revert #409 (test_utils workaround for broken Zygote - now working again)

    * disable broken Zygote AD test for ChainTransform

    Improved tests:

    * finer-grained testsets

    * add missing test cases to test_AD

    * replace test_FiniteDiff with test_AD(..., :FiniteDiff, ...)

    * remove code duplication

commit 3c49949
Author: Théo Galy-Fajou <[email protected]>
Date:   Wed Nov 24 18:32:19 2021 +0100

    Fix typo in valid_inputs error (#408)

    * Fix typo in valid_inputs error

    * Update src/utils.jl

    Co-authored-by: David Widmann <[email protected]>

    Co-authored-by: David Widmann <[email protected]>

commit 9955044
Author: st-- <[email protected]>
Date:   Wed Nov 24 18:55:18 2021 +0200

    Fix for Zygote 0.6.30 breaking our tests (#409)

    * restrict Zygote to <0.6.30

    * revert Zygote test restriction and add finer-grained testset

    * Update test/utils.jl

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * revert testset

    * mark test_broken

    * Use `@test_throws` instead of `@test_broken`

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
    Co-authored-by: David Widmann <[email protected]>

commit 33d64d1
Author: Théo Galy-Fajou <[email protected]>
Date:   Thu Nov 4 14:23:57 2021 +0100

    Add benchmarking CI (#399)

    * Add benchmark file

    * delete old benchmarks

    * Add github action

    * Add Project

    * Apply suggestions from code review

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

    * Missing end of line

    Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

commit 360ce10
Author: David Widmann <[email protected]>
Date:   Tue Nov 2 11:09:58 2021 +0100

    Update docstring of `GibbsKernel` (#395)

* change title

* Update manifest

* Formatter

* Some small updates

* Add BenchmarkTools

* Move Benchmarks to the correct manifest

* Formatter

* Fix missed comment

* Add ParameterHandling too

* Change gif embed

* Forgot #

* Formatter

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Compat and gif kwargs

* , for ;

* different commit msg

* Some more clarification

* Change display

* Final changes

* Apply suggestions from code review

Co-authored-by: st-- <[email protected]>

* Apply suggestions from code review

Co-authored-by: st-- <[email protected]>

* Address comments

* Missed one

* Manifest issue

* Apply suggestions from formatter

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Local formatter

* Delete loss description.

Co-authored-by: st-- <[email protected]>

* format

Co-authored-by: ST John <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distances 0.10.5 breaks tests
3 participants