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 evalpoly for no coefficients case #56706

Closed

Conversation

Sagar-Ramchandani
Copy link

This resolves #56699

I went with the route of returning zero(x) for evalpoly(x,p) instead of an error.
This is because according to me a naive implementation of evalpoly would use a counter initialized to zero and loop over the coefficients and would thereby return zero in case of no coefficients.

Please let me know if I have made a mistake in this pull request as this is my first PR.

@nsajko nsajko added the maths Mathematical functions label Nov 28, 2024
@@ -141,15 +144,18 @@ function evalpoly(z::Complex, p::Tuple)
_evalpoly(z, p)
end
end
evalpoly(z::Complex, p::Tuple{<:Any}) = p[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't unnecessarily increase diff size. Leave this line as it is.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for that. I have fixed it in the latest commit.

@nsajko
Copy link
Contributor

nsajko commented Nov 28, 2024

I'd suggest restricting this PR to fixing just the Tuple case. No sense in making things unnecessarily difficult.

@nsajko
Copy link
Contributor

nsajko commented Nov 28, 2024

Regarding the Tuple case, I'd suggest fixing the faulty method signatures as the starting point:

diff --git a/base/math.jl b/base/math.jl
index 16a8a54..c6acfca 100644
--- a/base/math.jl
+++ b/base/math.jl
@@ -91,7 +91,7 @@ julia> evalpoly(2, (1, 2, 3))
 17
 ```
 """
-function evalpoly(x, p::Tuple)
+function evalpoly(x, p::Tuple{Any, Vararg})
     if @generated
         N = length(p.parameters::Core.SimpleVector)
         ex = :(p[end])
@@ -116,7 +116,7 @@ function _evalpoly(x, p)
     ex
 end
 
-function evalpoly(z::Complex, p::Tuple)
+function evalpoly(z::Complex, p::Tuple{Any, Any, Vararg})
     if @generated
         N = length(p.parameters)
         a = :(p[end])

After that, adding a single method for empty tuples should suffice to give a friendly error message, without causing any ambiguity.

N = length(p)
N == 0 && return zero(z)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
N == 0 && return zero(z)
N == 0 && return zero(z) * zero(eltype(p))

for improved type-stability? Although that will throw if zero(eltype(p)) is undefined, e.g. if p == Any[], so not sure this is better.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your suggestion, but I personally think this change should not be made due to the possible undefined error in zero(eltype(p)) as you pointed out.

@Sagar-Ramchandani
Copy link
Author

I'd suggest restricting this PR to fixing just the Tuple case. No sense in making things unnecessarily difficult.

I am not sure I understand you correctly. Are you suggesting that I remove the changes made to the 'Vector' cases?

Regarding the Tuple case, I'd suggest fixing the faulty method signatures as the starting point:

I have fixed the faulty method signatures in the latest commit.

@Sagar-Ramchandani
Copy link
Author

I will be closing this PR because of a more general PR #56707

@Sagar-Ramchandani Sagar-Ramchandani deleted the sramchan/evalpoly branch November 28, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

evalpoly(x,()) gives unhelpful error.
3 participants