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

Implementing CSRK code in BSeries.jl #144

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

Sondar74
Copy link
Contributor

@Sondar74 Sondar74 commented Jun 26, 2023

This code is based on the paper "A characterization of energy-preserving methods and the construction of parallel integrators for Hamiltonian systems" (https://doi.org/10.48550/arXiv.1505.02537).

Added a struct in BSeries.jl called ContinuousStageRungeKutta and a function CSRK().

Example:

The energy-preserving 4x4 matrix given by Miyatake & Butcher (2015) is

M = [-6//5   72//5  -36//1  24//1;
        72//5  -144//5 -48//1  72//1;
        -36//1 -48//1   720//1 -720//1;
        24//1   72//1  -720//1  720//1]

Then, we calculate the bseries with the following code:

csrk = ContinuousStageRungeKuttaMethod(M)
series = bseries(csrk, 4)
TruncatedBSeries{RootedTree{Int64, Vector{Int64}}, Rational{Int64}} with 9 entries:
  RootedTree{Int64}: Int64[]      => 1//1
  RootedTree{Int64}: [1]          => 1//1
  RootedTree{Int64}: [1, 2]       => 1//2
  RootedTree{Int64}: [1, 2, 3]    => 1//6
  RootedTree{Int64}: [1, 2, 2]    => 1//3
  RootedTree{Int64}: [1, 2, 3, 4] => 1//24
  RootedTree{Int64}: [1, 2, 3, 3] => 1//12
  RootedTree{Int64}: [1, 2, 3, 2] => 1//8
  RootedTree{Int64}: [1, 2, 2, 2] => 1//4

@ranocha
Copy link
Owner

ranocha commented Jun 26, 2023

The failing tests come from a deprecation warning fixed in JuliaPy/PyCall.jl#1042. It will be fixed by #145

Copy link
Owner

@ranocha ranocha 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 for the PR! Now, everything seems to work. Please close the obsolete PRs.

  • Please add references and explain why you are doing what you are doing.
  • Please add tests.
  • Please check type stability (also in the tests).
  • Could you please describe an example of the new functionality in your opening post?
  • How is the performance behavior compared to the other implementations generating B-series we have so far?

src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated
Comment on lines 2060 to 2062
# we need variables to work with
variable1 = Sym(t)
variable2 = Sym(z)
Copy link
Owner

Choose a reason for hiding this comment

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

I do not really like that this introduces a hard dependency on SymPy.jl here (which is not covered in the import statements at the top of this file or Project.toml). If I remember correctly, @ketch reported some installation issues with some Julia packages build on Python packages. If possible, I would like to avoid such issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we work only with polynomials and just need to integrate, in principle everything can be done by working directly with the coefficients. This will make the code a bit harder to read but may also be faster.

src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated
Comment on lines 675 to 684
function bseries(csrk::ContinuousStageRungeKuttaMethod, order)
csrk = csrk.matrix
bseries(o) do t, series
if order(t) in (0, 1)
return one(csrk)
else
return elementary_differentials_csrk(csrk, t)
end
end
end
Copy link
Owner

Choose a reason for hiding this comment

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

Please add tests. This does not work correctly right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey, I'm working on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When working with the function one(), I have the problem that eltype(csrk) returns Any. How can I fix it?

Copy link
Owner

Choose a reason for hiding this comment

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

What is the csrk that you use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the csrk obtained from csrk = ContinuousStageRungeKuttaMethod(M), for a matrix M.

Copy link
Owner

@ranocha ranocha Jun 28, 2023

Choose a reason for hiding this comment

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

Use eltype(csrk.matrix) instead or define eltype(csrk::ContinuousStageRungeKuttaMethod) appropriately. The latter would be my preferred option, e.g.,

struct ContinuousStageRungeKuttaMethod{T, MatT <: AbstractMatrix{T}} <: RootedTrees.AbstractTimeIntegrationMethod
    A::MatT
end

Base.eltype(::ContinuousStageRungeKuttaMethod{T}) where {T} = T

Copy link
Owner

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

  • Please add tests with floating point coefficients.
  • Please add tests with symbolic coefficients using SymPy.jl, SymEngine.jl, and Symbolics.jl.

src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated Show resolved Hide resolved
src/BSeries.jl Outdated
Comment on lines 670 to 673
RootedTree{Int64}: [1, 2, 3] => 6004799503160661//36028797018963968
RootedTree{Int64}: [1, 2, 2] => 6004799503160661//18014398509481984
RootedTree{Int64}: [1, 2, 3, 4] => 6004799503160661//144115188075855872
RootedTree{Int64}: [1, 2, 3, 3] => 6004799503160661//72057594037927936
Copy link
Owner

Choose a reason for hiding this comment

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

Is this really correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, sorry: seems like there is something happening with the definition of V in

series = TruncatedBSeries{RootedTree{Int, Vector{Int}}, V}()
series[rootedtree(Int[])] = one(V)

so that, although the output of elementary_differentials_csrk(csrk, t) is correct, for some values it will throw an approximation to the original result instead of saving the value of elementary_differentials_csrk(csrk, t) Do you know how can I fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the numerical function N() solves this issue, but I don't know if it has limitations for future applications of this code (since this is the first time I'm working with this function).

Copy link
Owner

@ranocha ranocha Jun 28, 2023

Choose a reason for hiding this comment

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

Let's see. First, please add more tests etc. - and fix the CI problems. Then, possible problem can show up already

src/BSeries.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Sondar74 and others added 8 commits June 28, 2023 10:33
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
@Sondar74
Copy link
Contributor Author

  • Please add tests with floating point coefficients.
  • Please add tests with symbolic coefficients using SymPy.jl, SymEngine.jl, and Symbolics.jl.

Seems like a symbolic matrix with inputs from SymEngine is not compatible with the PolynomialsA function.

@ranocha
Copy link
Owner

ranocha commented Jun 28, 2023

  • Please add tests with floating point coefficients.
  • Please add tests with symbolic coefficients using SymPy.jl, SymEngine.jl, and Symbolics.jl.

Seems like a symbolic matrix with inputs from SymEngine is not compatible with the PolynomialsA function.

That doesn't surprise me since SymPy.jl is probably not compatible with SymEngine.jl etc.

test/runtests.jl Show resolved Hide resolved
test/runtests.jl Outdated
Comment on lines 2090 to 2094
# Create symbolic matrix
Minv = [a 1//2 b 1//6;
1//2 1//4 1//6 1//8;
b 1//6 c 1//10;
1//6 1//8 1//10 1//12]
Copy link
Owner

Choose a reason for hiding this comment

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

Please check your comments everywhere. This is not a symbolic matrix since you set the coefficients above to fixed floating point values. Where did you get these special choices from? Please add some references

Copy link
Contributor Author

@Sondar74 Sondar74 Jun 28, 2023

Choose a reason for hiding this comment

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

Okey, I'll fix the comments. As for the values, I took them from the project I am working on with @ketch, I calculated them.
Since it is the same matrix from Miyatake & Butcher, I will add this reference too.

Comment on lines +2099 to +2103
# Generate the bseries up to order 4
order = 4
series = bseries(csrk, order)
expected_coefficients = [1.0 ,1.0, 0.5000000000000002, 0.16666666666666666, 0.3333333333333336, 0.04166666666666661, 0.0833333333333332, 0.12500000000000003, 0.2500000000000003]
@test collect(values(series)) == expected_coefficients
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment about the order, energy-preserving property etc. as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the is_energy_preserving function will not necesary work accurately for Floating coefficients: for example, if we wanted to express 1/3 in floating we may need to cut it in some point at some decimal place, so there would be loss of information and is_energy_preserving would return false

Copy link
Owner

Choose a reason for hiding this comment

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

Did you test it?

test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
# Generate the bseries up to order 4
order = 3
series = bseries(csrk, order)
expected_coefficients = [1,1, 25*x^2/32 + 5*x*y/4 + y^2/2, 1105*x^3/2304 + 671*x^2*y/576 + 545*x*y^2/576 + 37*y^3/144, 125*x^3/192 + 25*x^2*y/16 + 5*x*y^2/4 + y^3/3]
Copy link
Owner

Choose a reason for hiding this comment

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

Did you compute this by hand? How did you arrive at these coefficients?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just copied from the output of the working code

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm... So it is just a regression test - assuming that the current implementation is correct. Could you please also add real tests using examples with known behavior? IIRC, Miyatake & Butcher (2015) have some examples with free coefficients in their paper. You could express these free coefficients as symbolic variables and check whether the resulting B-series of the CSRK method

  • has the correct order_of_accuracy
  • is_energy_preserving

test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
Sondar74 and others added 5 commits June 28, 2023 17:06
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
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

3 participants