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

don't use Val with ntuple when the length isn't a constant #525

Merged
merged 1 commit into from
Aug 20, 2023
Merged

don't use Val with ntuple when the length isn't a constant #525

merged 1 commit into from
Aug 20, 2023

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Aug 20, 2023

Helps type inference.

Before:

julia> using Test, JET, Polynomials

julia> p = ImmutablePolynomial{Bool, :x, 1}((true,))
ImmutablePolynomial(true)

julia> @inferred coeffs(p)
ERROR: return type Tuple{Bool} does not match inferred return type Any

After:

julia> using Test, JET, Polynomials

julia> p = ImmutablePolynomial{Bool, :x, 1}((true,))
ImmutablePolynomial(true)

julia> @inferred coeffs(p)
ERROR: return type Tuple{Bool} does not match inferred return type Tuple{Vararg{Bool}}

Helps type inference.

Before:
```julia-repl
julia> using Test, JET, Polynomials

julia> p = ImmutablePolynomial{Bool, :x, 1}((true,))
ImmutablePolynomial(true)

julia> @inferred coeffs(p)
ERROR: return type Tuple{Bool} does not match inferred return type Any
```

After:
```julia-repl
julia> using Test, JET, Polynomials

julia> p = ImmutablePolynomial{Bool, :x, 1}((true,))
ImmutablePolynomial(true)

julia> @inferred coeffs(p)
ERROR: return type Tuple{Bool} does not match inferred return type Tuple{Vararg{Bool}}
```
@nsajko
Copy link
Contributor Author

nsajko commented Aug 20, 2023

This also seems to wholly prevent run time dispatch for coeffs(::ImmutablePolynomial)! At least for small lengths? Not sure how this is possible.

Before:

julia> using Test, JET, Polynomials

julia> p = ImmutablePolynomial{Bool, :x, 7}((true,true,true,true,true,true,true))
ImmutablePolynomial(true + x + x^2 + x^3 + x^4 + x^5 + x^6)

julia> @report_opt coeffs(p)
═════ 1 possible error found ═════
┌ coeffs(p::ImmutablePolynomial{Bool, :x, 7}) @ Polynomials /home/nsajko/tmp/jl/Polynomials.jl/src/polynomial-container-types/immutable-dense-polynomial.jl:212
│┌ trim_trailing_zeros!!(cs::NTuple{7, Bool}) @ Polynomials /home/nsajko/tmp/jl/Polynomials.jl/src/polynomial-container-types/immutable-dense-polynomial.jl:101
││ runtime dispatch detected: Polynomials.ntuple(%92::Base.Fix1{typeof(getindex), NTuple{7, Bool}}, %94::Val)::Any
│└────────────────────

After:

julia> p = ImmutablePolynomial{Bool, :x, 7}((true,true,true,true,true,true,true))
ImmutablePolynomial(true + x + x^2 + x^3 + x^4 + x^5 + x^6)

julia> @report_opt coeffs(p)
No errors detected

@codecov
Copy link

codecov bot commented Aug 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.28% 🎉

Comparison is base (5f5ba86) 75.31% compared to head (13ff12c) 76.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #525      +/-   ##
==========================================
+ Coverage   75.31%   76.59%   +1.28%     
==========================================
  Files          34       35       +1     
  Lines        3528     3739     +211     
==========================================
+ Hits         2657     2864     +207     
- Misses        871      875       +4     
Files Changed Coverage Δ
...mial-container-types/immutable-dense-polynomial.jl 88.12% <100.00%> (+0.70%) ⬆️

... and 26 files with indirect coverage changes

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

@jverzani
Copy link
Member

Thanks again!!

@jverzani jverzani merged commit aa9bec7 into JuliaMath:master Aug 20, 2023
20 of 21 checks passed
@nsajko nsajko deleted the noval branch August 20, 2023 13:04
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.

2 participants