Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add check_args and drop 1.3 #499

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

Open
theogf wants to merge 10 commits into master
base: master
Choose a base branch
Loading
from tgf/checkargs
Open

Add check_args and drop 1.3 #499

theogf wants to merge 10 commits into master from tgf/checkargs

Conversation

@theogf
Copy link
Member

@theogf theogf commented Apr 18, 2023

Summary
Similarly to Distributions.jl we do not necessarily want to check for argument correctness all the time.

Proposed changes

This adds a check_args keyword to all constructors that require it to deactivate the check for the correctness for the arguments.
Additionally this drops 1.3 for 1.6 because I am lazy to write kwarg=kwarg (appears in 1.5 or 1.6 can't remember0

  • ...

What alternatives have you considered?
We could define a reparametrization of the parameters but we already discussed that it should be the duty of the user to do this.

Breaking changes
None! Crazy right?

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Approved subject to CI etc. I'd also like an approval from either @devmotion or @st-- before merging

Copy link

codecov bot commented Apr 18, 2023
edited
Loading

Codecov Report

❌ Patch coverage is 73.07692% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.83%. Comparing base (8746034) to head (0c8185a).
⚠️ Report is 38 commits behind head on master.

Files with missing lines Patch % Lines
src/utils.jl 12.50% 21 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@
## master #499 +/- ##
==========================================
- Coverage 94.25% 92.83% -1.42% 
==========================================
 Files 52 52 
 Lines 1374 1396 +22 
==========================================
+ Hits 1295 1296 +1 
- Misses 79 100 +21 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

I'm a bit worried that this might cause performance regressions, or at least is suboptimal, based on the fixes that were needed in Distributions: JuliaStats/Distributions.jl#1492

willtebbutt reacted with thumbs up emoji

Copy link
Member

Good point @devmotion . @theogf could you check some simple examples with Zygote?

Copy link
Member Author

theogf commented Apr 18, 2023

I'm a bit worried that this might cause performance regressions, or at least is suboptimal, based on the fixes that were needed in Distributions: JuliaStats/Distributions.jl#1492

Then shouldn't I just reuse the @check_args from Distributions.jl ?

willtebbutt reacted with thumbs up emoji

theogf and others added 6 commits April 18, 2023 12:54
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>
Copy link
Member Author

theogf commented Apr 18, 2023

@devmotion do you think that would be enough?

Copy link
Member

Did you do compare performance with Zygote?

Copy link
Member Author

theogf commented Apr 19, 2023

Here are the benchmarks:

using Zygote
using BenchmarkTools
using KernelFunctions
x = [3.0]
macro old_check_args(K, param, cond, desc=string(cond))
 quote
 if !($(esc(cond)))
 throw(
 ArgumentError(
 string(
 $(string(K)),
 ": ",
 $(string(param)),
 " = ",
 $(esc(param)),
 " does not ",
 "satisfy the constraint ",
 $(string(desc)),
 ".",
 ),
 ),
 )
 end
 end
end
struct OldLinearKernel{Tc<:Real} <: KernelFunctions.SimpleKernel
 c::Vector{Tc}
 function OldLinearKernel(c::Real)
 @old_check_args(LinearKernel, c, c >= zero(c), "c ≥ 0")
 return new{typeof(c)}([c])
 end
end
function f(x)
 k = LinearKernel(;c = x[1])
 sum(k.c)
end
function g(x)
 k = LinearKernel(;c = x[1], check_args=false)
 sum(k.c)
end
function h(x)
 k = OldLinearKernel(x[1])
 sum(k.c)
end
@btime Zygote.gradient($f, $x) # 15.980 μs (150 allocations: 5.89 KiB)
@btime Zygote.gradient($g, $x) # 13.853 μs (142 allocations: 5.72 KiB)
@btime Zygote.gradient($h, $x) # 4.700 μs (51 allocations: 1.89 KiB)
devmotion reacted with confused emoji

Copy link
Member

That's a quite noticeable regression. Do we know what exactly causes it?

Copy link
Member Author

theogf commented Apr 19, 2023
edited
Loading

For completeness I added the constructor

 function OldLinearKernel(c::Real; check_args=true)
 check_args && @old_check_args(LinearKernel, c, c >= zero(c), "c ≥ 0")
 return new{typeof(c)}([c])
 end

And these are the results

function h(x)
 k = OldLinearKernel(x[1])
 sum(k.c)
end
function i(x)
 k = OldLinearKernel(x[1]; check_args=false)
 sum(k.c)
end
@btime Zygote.gradient($h, $x) # 6.086 μs (65 allocations: 2.58 KiB)
@btime Zygote.gradient($i, $x) # 10.404 μs (91 allocations: 3.38 KiB)

Copy link
Member Author

theogf commented Feb 17, 2024

Looking back at this, how about using CheckArgs.jl ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@willtebbutt willtebbutt willtebbutt approved these changes

@devmotion devmotion Awaiting requested review from devmotion

@st-- st-- Awaiting requested review from st--

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /