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

Updates due to JuliaDiff/TaylorSeries.jl#361 #194

Merged
merged 16 commits into from
Jul 22, 2024
Merged

Updates due to JuliaDiff/TaylorSeries.jl#361 #194

merged 16 commits into from
Jul 22, 2024

Conversation

PerezHz
Copy link
Owner

@PerezHz PerezHz commented Jul 12, 2024

This PR contains updates which go together with JuliaDiff/TaylorSeries.jl#361

EDIT: this PR depends also on JuliaDiff/TaylorSeries.jl#364

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 20, 2024

Tests were actually passing modulo coverage, except for the last commit, but cannot reproduce the issue locally, so... I'm re-running tests and see if it's maybe a CI issue

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 20, 2024

After re-running tests, they are passing except for Julia 1.6 on the macos platform... does look like a CI issue?

@lbenet
Copy link
Collaborator

lbenet commented Jul 20, 2024

After re-running tests, they are passing except for Julia 1.6 on the macos platform... does look like a CI issue?

Also windows in Julia 1.6 is failing. I doubt it is a CI issue, it should be deterministic, right?

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 20, 2024

Also windows in Julia 1.6 is failing. I doubt it is a CI issue, it should be deterministic, right?

So you're saying maybe it's an initialization issue?

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 20, 2024

Windows and MacOS failures on Julia 1.6 look pretty consistent, whereas all the other versions and platforms tests passing also look consistent. Probably it's then a sqrt issue which is somehow only visible on Julia 1.6 on Windows and MacOS but not on Ubuntu?

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 21, 2024

Update: managed to reproduce the error on julia 1.6 (macos)

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 21, 2024

Turns out the issue in julia1.6 can be traced back to rounding off issues in powers of Float64:

julia> 0.2029886550675951^3 # integer power
 0.008364024538422981

julia> 0.2029886550675951^3.0 # float power
 0.00836402453842298

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 21, 2024

Pushed a fix to TaylorSeries...

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 21, 2024

... et voilà!

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 21, 2024

In a nutshell, since some allocations were incurred in the preamble of parsed jetcoeffs! methods, in one of the last commits I removed the preamble in favor of using mutating methods for 0-th order, modifying the recursion loop accordingly. For power operations such as x = r^3 the preamble did a float conversion x = Taylor1(constant_term(r) ^ float(constant_term(3)), order). The equivalent conversion in pow! was missing, and hence different results were obtained by parsed and non-parsed jetcoeffs! methods for Julia 1.6 (and my guess is that this issue somehow did not make it to the official Linux x64 binaries, so the issue did not appear there).

@lbenet
Copy link
Collaborator

lbenet commented Jul 21, 2024

In a nutshell, since some allocations were incurred in the preamble of parsed jetcoeffs! methods, in one of the last commits I removed the preamble in favor of using mutating methods for 0-th order, modifying the recursion loop accordingly. For power operations such as x = r^3 the preamble did a float conversion x = Taylor1(constant_term(r) ^ float(constant_term(3)), order). The equivalent conversion in pow! was missing, and hence different results were obtained by parsed and non-parsed jetcoeffs! methods for Julia 1.6 (and my guess is that this issue somehow did not make it to the official Linux x64 binaries, so the issue did not appear there).

Thanks for checking and solving this!

@lbenet
Copy link
Collaborator

lbenet commented Jul 21, 2024

Registration of Taylor series v0.18.0 is on its way... 10-15 minutes are required 😄

@lbenet
Copy link
Collaborator

lbenet commented Jul 21, 2024

v0.18.0 is released!

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 21, 2024

v0.18.0 is released!

Many thanks! I'll re-trigger tests here!

@PerezHz PerezHz marked this pull request as ready for review July 21, 2024 20:57
src/TaylorIntegration.jl Outdated Show resolved Hide resolved
@PerezHz
Copy link
Owner Author

PerezHz commented Jul 21, 2024

With TaylorSeries v0.18.0 released this is now ready for review

@lbenet
Copy link
Collaborator

lbenet commented Jul 22, 2024

With TaylorSeries v0.18.0 released this is now ready for review

I'll try to review it this afternoon... sorry for the delay.

Copy link
Collaborator

@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 @PerezHz. My only suggestion is to bump a new (minor or patch?) version. Aside from that, LGTM!

@PerezHz
Copy link
Owner Author

PerezHz commented Jul 22, 2024

Thanks a lot @PerezHz. My only suggestion is to bump a new (minor or patch?) version. Aside from that, LGTM!

Alright, bumped the patch version and will merge afterwards, thanks!

@PerezHz PerezHz merged commit e32cd4d into main Jul 22, 2024
14 checks passed
@PerezHz PerezHz deleted the jp/ts-pr-361 branch July 22, 2024 20:49
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.

2 participants