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.

Sign up for GitHub

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

Jump to bottom

Extend evaluate on AbstractArray, instead of Array #167

Merged
merged 3 commits into from
May 30, 2018
Merged

Extend evaluate on AbstractArray, instead of Array #167

merged 3 commits into from
May 30, 2018

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented May 17, 2018

This allows for evaluations using IntervalBoxes.

@coveralls
Copy link

coveralls commented May 17, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 98.086% when pulling 364afe0 on lb/evaluate_AbstArray into c0d4859 on master.

dpsanders
dpsanders reviewed May 17, 2018
View reviewed changes
src/evaluate.jl Outdated
@@ -264,6 +264,10 @@ function evaluate(a::TaylorN{T}, vals::NTuple{N,S}) where
return sum( sort!(suma, by=abs2) )
end

# This should allow to evaluate IntervalBoxes appropriately
evaluate(a::TaylorN{T}, vals::AbstractArray) where
Copy link
Contributor

Choose a reason for hiding this comment

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

After JuliaIntervals/IntervalArithmetic.jl#152, IntervalBox will no longer be an AbstractArray.

However, as far as I can see, there's no reason to restrict this to AbstractArray -- you don't need to type annotate vals at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot! Good catch! I'll try your suggestion.

@lbenet
Copy link
Member Author

lbenet commented May 17, 2018

Thanks again for the suggestion.

@dpsanders
Copy link
Contributor

Now that I think about it, it would be best to avoid the splat (...) if possible, for efficiency.
Can you just treat the argument as an iterator with a known length in the original evaluate function?
i.e. start by checking the length of the argument and then suppose that you can index into it?

@lbenet
Copy link
Member Author

lbenet commented May 18, 2018

Thanks for the suggestion. I think it is worth considering it, though I am not sure how.

The original evaluate function (for TaylorN, which is the case of interest in this PR) takes a TaylorN and a NTuple, and is implemented through a for of known length here. So my guess is that it is coded in a way you propose, but maybe in a primitive way. Some of the remaining methods do splat vectors, or treat chains of parameters as a proper ntuple by splatting, simply to call the original method. I am not sure if there is a high penalty on doing that.

@dpsanders
Copy link
Contributor

To be honest I just have the general idea that "splatting is slow", but I guess it would require actual benchmarks to check that. Let's just leave it for now.

@lbenet
Copy link
Member Author

lbenet commented May 18, 2018

Agree, let's leave it for now.

@lbenet
Copy link
Member Author

lbenet commented May 20, 2018

Do you agree to go ahead and merge this?

@lbenet lbenet merged commit c18f779 into master May 30, 2018
@lbenet lbenet deleted the lb/evaluate_AbstArray branch June 27, 2018 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpsanders dpsanders dpsanders left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lbenet @coveralls @dpsanders