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

Improve inference in derivative/integrate #514

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Jul 21, 2023

On master

julia> p = fromroots(Polynomial, Float64[1,1,2])
Polynomial(-2.0 + 5.0*x - 4.0*x^2 + 1.0*x^3)

julia> using Test

julia> @inferred derivative(p)
ERROR: return type Polynomial{Float64, :x} does not match inferred return type Polynomial{Float64}
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] top-level scope
   @ REPL[4]:1

julia> @inferred integrate(p)
ERROR: return type Polynomial{Float64, :x} does not match inferred return type Polynomial{Float64}
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] top-level scope
   @ REPL[5]:1

This PR fixes these. The inference test is skipped for ImmutablePolynomial, as the length doesn't appear to be inferred (and I see several open issues regarding this, so I haven't bothered looking into it).

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2de280b) 79.78% compared to head (da9fe22) 79.78%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #514   +/-   ##
=======================================
  Coverage   79.78%   79.78%           
=======================================
  Files          26       26           
  Lines        3216     3216           
=======================================
  Hits         2566     2566           
  Misses        650      650           
Impacted Files Coverage Δ
src/polynomials/standard-basis.jl 80.36% <100.00%> (ø)

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

@jishnub
Copy link
Member Author

jishnub commented Jul 21, 2023

The invalidation test failure is because of an upstream (Pkg) issue

The test failure on v1.6 for ControlSystems is perhaps unrelated, and the repo doesn't seem to run CI tests on v1.6 anyway

@jverzani
Copy link
Member

Thanks!

In #513 there are some timings where you can see the impact of this not being inferred. That PR addresses the ImmutablePolynomial case.

@jverzani jverzani merged commit f4d90d9 into JuliaMath:master Jul 21, 2023
@jishnub jishnub deleted the derivintinfer branch July 21, 2023 12:02
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