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

Upgrade TaylorSeries, TaylorIntegration and TaylorModels #808

Merged
merged 7 commits into from
Apr 5, 2024

Conversation

lbenet
Copy link
Collaborator

@lbenet lbenet commented Mar 14, 2024

Closes #804

@lbenet
Copy link
Collaborator Author

lbenet commented Mar 14, 2024

I don't understand why the tests are broken; locally things behave better... Any advice?

Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

We need to first propagate the new version through the dependencies. That would be at least LazySets, RangeEnclosures, and Flowstar. I started the process, but it will take some time.

FYI, to reproduce, you can use using TestEnv; TestEnv.activate(); to activate the test environment locally. This way you also do not have to add the package to the test environment.

test/Project.toml Outdated Show resolved Hide resolved
test/Project.toml Outdated Show resolved Hide resolved
test/reachsets/reachsets.jl Outdated Show resolved Hide resolved
@schillic
Copy link
Member

We need to first propagate the new version through the dependencies. That would be at least LazySets, RangeEnclosures, and Flowstar. I started the process, but it will take some time.

I just ran the tests with the master version for these packages and then tests pass 🎉

@schillic
Copy link
Member

There is an error with evaluate(X, tdom) where X is a Taylor model (see the test log). Looks like a bug in the other packages.
BoundsError: attempt to access 1-element Vector{HomogeneousPolynomial{IntervalArithmetic.Interval{Float64}}} at index [2]

@lbenet
Copy link
Collaborator Author

lbenet commented Mar 15, 2024

There is an error with evaluate(X, tdom) where X is a Taylor model (see the test log). Looks like a bug in the other packages. BoundsError: attempt to access 1-element Vector{HomogeneousPolynomial{IntervalArithmetic.Interval{Float64}}} at index [2]

I'll have a look...

@lbenet
Copy link
Collaborator Author

lbenet commented Mar 15, 2024

The problem is that evaluate (in TaylorSeries) is picking up the wrong method, so it is an inference method. I'll patch TaylorSeries this afternoon...

@schillic
Copy link
Member

I do not understand why the documentation build does not find the new version.
(I know that it takes some time for packages to be available after a release, but we already skip this delay via JULIA_PKG_SERVER: '' in the build script. I suspected that we only skip it for the tests, but we also do for the documentation.)
Maybe we can try again tomorrow.

@schillic
Copy link
Member

schillic commented Mar 16, 2024

It still does not work. My current suspicion is that this is because of the cache that is being used by the build. Maybe when you push the new version for the small docstring changes it will skip the cache. Otherwise we can just build it locally and ignore it here.

EDIT: Locally the build runs through. So I propose we ignore it here.

@schillic
Copy link
Member

schillic commented Mar 17, 2024

Here is the current status wrt. known models:

  • ReachabilityAnalysis: All models still work. This includes the Brusselator which used TMJets20.
  • ClosedLoopReachability: All models still work.
  • ARCH-COMP AINNCS: All models still work.
  • ARCH-COMP NLN: All models except one still work. The one that does not work anymore is Spacecraft, which used TMJets20.
    • While looking at the model, I found that the version used here in ReachabilityAnalysis is inconsistent. It contains a new update but does not contain an older update. I opened two alternative PRs: Adapt Spacecraft settings to ARCH-COMP2020 #814 to undo the new update and Adapt Spacecraft settings to ARCH-COMP2022 #815 to add the older update.
    • I have not yet investigated whether different parameter settings with the other algorithms can be used.
      EDIT: I found a setting where the new algorithm works (was as easy as raising orderT by 1). However, the algorithm is a lot slower (130 seconds vs. 24 seconds).

@schillic
Copy link
Member

schillic commented Mar 17, 2024

I found a "fix" for the spacecraft benchmark, but it is a lot slower (see the previous comment). Still, I think the benefit of being able to use the new versions again outweighs this cost for me. @mforets, what is your opinion?

I would prefer to not use the algorithm names 1 and 2 and rather unify the structs and pass a new option. But I can take a look at this in a later step. So we should not make a release immediately.

So from my side, this PR is good to go. I will mark it as ready.

@schillic schillic marked this pull request as ready for review March 17, 2024 22:44
@lbenet
Copy link
Collaborator Author

lbenet commented Mar 17, 2024

Thanks @schillic for looking onto the performance consequences. I'll try to have a look onto that tomorrow.

I would prefer to not use the algorithm names 1 and 2 and rather unify the structs and pass a new option. But I can take a look at this in a later step. So we should not make a release immediately.

I like the proposal of making a more uniform API; yet, there are some differences wrt the parameters that each method uses, which may add some complications.

@schillic
Copy link
Member

schillic commented Apr 5, 2024

@lbenet I will merge and then do some renaming in a follow-up PR -> #818.

@schillic schillic merged commit 90a0036 into master Apr 5, 2024
6 checks passed
@schillic schillic deleted the lb/upgradeTS-TI branch April 5, 2024 18:11
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.

On TMJets
3 participants