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

Add evaluation method for arrays of TaylorN #364

Merged
merged 10 commits into from
Jul 21, 2024

Conversation

PerezHz
Copy link
Contributor

@PerezHz PerezHz commented Jul 17, 2024

This is still WIP, but the idea here is to improve allocations when substituting a tuple of TaylorNs (let's call it vals) into each element of an array arr of TaylorN. So far I haven't managed to get rid of all allocations, but instead the number of allocations is constant, no matter the size of arr.

EDIT: no longer WIP, now ready for review.

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 18, 2024

Rebased to current master

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 18, 2024

The proposed method works as follows:

x = set_variables("x", order=2, numvars=4)
# helper function to fill TaylorN with random coefficients
function radntn!(y)
    for k in eachindex(y)
        for l in eachindex(y[k])
            y[k][l] = randn()
        end
    end
    nothing
end
y = zero(x[1])
radntn!(y)
n = 10
v = [zero(x[1]) for _ in 1:n] # array of TaylorN to evaluate
r = [zero(x[1]) for _ in 1:n] # output vector
radntn!.(v)
x1 = randn(4) .+ x # TaylorN variables to substitute into `v`
@time TaylorSeries.evaluate!(v, (x1...,), r) # new evaluate! method
@time r2 = evaluate.(v, Ref(x1)) # current mechanism (could also do it via map!)
r == r2 # true

@LuEdRaMo can you check if this works faster than the current workaround while maintaining essentially the same results?

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 18, 2024

One thing to note is that as currently proposed the new evaluate! method accumulates the result in the output vector (r in the example). Depending on the application the user might not want to accumulate the result; in this case I can modify things to optionally do zero!(r) at the start of the new evaluate! method depending on the value of an accumulate::Bool kwarg.

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 18, 2024

Also, currently this evaluate! method initializes a cache, so if this method is going to be called many times probably it makes sense to initialize the cache outside the method. For this, we could consider adding a struct, say EvaluateCache, and passing it eg as a kwarg to optionally allocate it depending on whether the user passes an initialized EvaluateCache or not.

@LuEdRaMo
Copy link
Contributor

@LuEdRaMo can you check if this works faster than the current workaround while maintaining essentially the same results?

In my NEOs.jl script for imminent impactors:

# TS: master
43.177703 seconds (458.16 M allocations: 127.817 GiB, 35.71% gc time)

# TS: jp/evaluate-tn
21.322079 seconds (209.04 M allocations: 25.796 GiB, 18.86% gc time)

However, NEOs.jl requires a TaylorIntegration.jl update to adapt to the recent changes in pow!.

@lbenet
Copy link
Member

lbenet commented Jul 20, 2024

This looks very promising! Regarding TaylorIntegration, I hope to find time and work on that this afternoon...

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 20, 2024

In my NEOs.jl script for imminent impactors:

How many times (approx) are you calling the evaluate! method from this PR in your script?

However, NEOs.jl requires a TaylorIntegration.jl update to adapt to the recent changes in pow!.

For this, you can try to use the following setup in your environment:

TaylorSeries.jl branch jp/evaluate-tn from my fork (i.e., the branch used in this PR)
TaylorIntegration.jl branch jp/ts-pr-361
NEOS.jl branch jp/ts-pr-361

Should work in principle, but do let me know otherwise!

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 20, 2024

This looks very promising! Regarding TaylorIntegration, I hope to find time and work on that this afternoon...

I opened PerezHz/TaylorIntegration.jl#194 to do the corresponding updates coming from #361, maybe we can join efforts there!

@lbenet
Copy link
Member

lbenet commented Jul 20, 2024

I opened PerezHz/TaylorIntegration.jl#194 to do the corresponding updates coming from #361, maybe we can join efforts there!

Thanks for opening that PR. I'll have a look and try to ammend @taylorize; I think we can do it with a clever if...

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 20, 2024

Should work in principle, but do let me know otherwise!

Actually tests are not passing in PerezHz/TaylorIntegration.jl#194 so use with caution; I'm having a look now 🤔

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 20, 2024

I think we can do it with a clever if...

Clever ifs are the best! 😄

@PerezHz PerezHz changed the title WIP: add evaluation method for arrays of TaylorN Add evaluation method for arrays of TaylorN Jul 21, 2024
@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 21, 2024

One thing to note is that as currently proposed the new evaluate! method accumulates the result in the output vector (r in the example). Depending on the application the user might not want to accumulate the result; in this case I can modify things to optionally do zero!(r) at the start of the new evaluate! method depending on the value of an accumulate::Bool kwarg.

A small precision/update on this: I updated evaluate! such that it zero!s all non-zero elements of the destination array. This seems the most common use case, at least for the time being, and makes sense for a first release (we can always modify later if needed). So in this example it's not longer needed for the user to call TaylorSeries.zero! first.

@PerezHz
Copy link
Contributor Author

PerezHz commented Jul 21, 2024

This is now ready for review @lbenet

Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

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

Thanks a lot! LGTM! I'm merging this and tagging the new release.

@lbenet lbenet merged commit e5b4989 into JuliaDiff:master Jul 21, 2024
13 checks passed
@lbenet
Copy link
Member

lbenet commented Jul 21, 2024

Merged!

@PerezHz PerezHz deleted the jp/evaluate-tn branch July 21, 2024 21:00
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.

3 participants