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 product of TaylorModelNs #31

Closed
wants to merge 2 commits into from
Closed

Improve product of TaylorModelNs #31

wants to merge 2 commits into from

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Feb 10, 2019

This improves the product of two TaylorModelN objects. It avoids duplicating the order for TaylorN, which avoids some allocations.

@lbenet
Copy link
Member Author

lbenet commented Feb 10, 2019

I'll try to improve the bounds of the result; one test needed to be corrected.

@lbenet
Copy link
Member Author

lbenet commented Feb 10, 2019

This should fix #29

Copy link
Member

@dpsanders dpsanders left a comment

Choose a reason for hiding this comment

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

It's hard to keep track of the orders of everything. Can't we just fix the order of everybody?

src/arithmetic.jl Show resolved Hide resolved

# We evaluate each term of the product of coefficients at `aux`
Δnegl = zero(Δ)
for order = order_max+1:order_prod
Copy link
Member

Choose a reason for hiding this comment

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

Why does this start at order_max?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am correcting that, it should be orderTS. The idea, as I say above, is to compute the resulting polynomial up to orderTS, and from there bound whatever is not included in the polynomial part.

@lbenet
Copy link
Member Author

lbenet commented Feb 10, 2019

It's hard to keep track of the orders of everything. Can't we just fix the order of everybody?

It may be simpler, indeed, but the user is not forced to do that.

@lbenet
Copy link
Member Author

lbenet commented Feb 10, 2019

Just pushed a new commit which improves further (but not as significantly as before) the already improved allocations and timings.

@lbenet
Copy link
Member Author

lbenet commented Feb 10, 2019

When used in the context of validated integration, this PR leads to ~ 3x regression ...

@dpsanders
Copy link
Member

My idea was that we could just define arithmetic etc. operations only when the two Taylor models have the same order (which can be encoded as a type parameter) to simplify the code.

orderini = order - orderTS
for inda = orderini:order_a
indb = order - inda
# Δnegl += evaluate(apol[inda], aux) * evaluate(bpol[indb], aux)
Copy link
Member

Choose a reason for hiding this comment

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

You could try evaluating all of these bounds once before the loop (and storing them in new arrays), and then just indexing into the arrays inside the for loops. This should be much faster.

What I had done previously was store these bounds in the TaylorModel object itself when it is created (so that they are calculated exactly once in the history of the object).

@lbenet
Copy link
Member Author

lbenet commented Feb 11, 2019

I've been playing with the last two commits together with #18 and the van der Pol and Lotka-Volterra systems. As I noted above, there is a marked speed-down in comparison with what we have in #18 alone.

You could try evaluating all of these bounds once before the loop (and storing them in new arrays), and then just indexing into the arrays inside the for loops. This should be much faster.

What I had done previously was store these bounds in the TaylorModel object itself when it is created (so that they are calculated exactly once in the history of the object).

I guess that the problem is related with the big amount of evaluations required, which happen to be related with ^. Could you remind me which branch in IntervalArithmetic does this faster? In any case, note that I only evaluate the truncated part of the polynomial, coefficient by coefficient; in master this is done directly, because that part is computed explicitly.

@dpsanders
Copy link
Member

It's the fastpowers branch of IntervalArithmetic.jl. There is no question that we should be using that branch! It should be around a factor of 4 speedup (at least)?

@dpsanders
Copy link
Member

Note that if we use normalised polynomials then it should be possible to do evaluations without using powers at all!

@lbenet
Copy link
Member Author

lbenet commented Feb 11, 2019

It's the fastpowers branch of IntervalArithmetic.jl. There is no question that we should be using that branch! It should be around a factor of 4 speedup (at least)?

Thanks! I'll play using that branch and report things back here.

Note that if we use normalised polynomials then it should be possible to do evaluations without using powers at all!

I agree; the question is if that normalization should be -1..1 or 0..1. For the latter case, irrespective of the power we always get the same answer; for the former case, the result will be either 0..1 if the power is even, or -1..1 if the power is odd.

@lbenet
Copy link
Member Author

lbenet commented Feb 11, 2019

Another possibility, which is the one I'm pursuing now, is to work with TaylorModelsN{N, Float64, Float64}, which corresponds to have polynomial coefficients of type Float64. I think this is what Bertz and Makino do, and probably others too.

@lbenet
Copy link
Member Author

lbenet commented Jun 29, 2020

Closing; see #81

@lbenet lbenet closed this Jun 29, 2020
@lbenet lbenet deleted the lb/mult_TMNs branch November 16, 2021 22: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.

None yet

2 participants