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

Fix docs build #31

Merged
merged 11 commits into from
Jan 15, 2025
Merged

Fix docs build #31

merged 11 commits into from
Jan 15, 2025

Conversation

JonasKoziorek
Copy link
Contributor

One of the algorithms stopped converging for examples in the docs. I changed the initial guesses and precision. This must have resulted from some changes in other libraries, probably NonlinearSolve.

@JonasKoziorek
Copy link
Contributor Author

Strange, I pass the tests locally.

@Datseris
Copy link
Member

Datseris commented Jan 2, 2025

Any way you can fix the tests? change tolerances or something? Docs wont build if tests fail.

Thanks for the PR!

@JonasKoziorek
Copy link
Contributor Author

I will play around with parameters and see if it passes the tests.

@JonasKoziorek
Copy link
Contributor Author

Any tips on how to debug why the tests run locally and not here?

@Datseris
Copy link
Member

Datseris commented Jan 8, 2025

Can you tell me what is going wrong with the test? What is it supposed to be testing and what it finds instead?

@JonasKoziorek
Copy link
Contributor Author

res = periodic_orbits(ds, alg, igs)
@test length(res) == 2

Given two initial guesses, the algorithm finds an orbit only for one of the guesses.
Maybe after the update of NonlinearSolve the SciML solver doesn't converge to the same result for the given initial guess. (11063cb#diff-72ed386c2a0cd1d23c0968297e70023ed98c22490d146dd89fc91f48369bad4dL18)

@Datseris
Copy link
Member

Datseris commented Jan 9, 2025

Can you just check out the branch, run the initial conditions and see if you get the orbit you should, otherwise change the guess?

@Datseris
Copy link
Member

Datseris commented Jan 9, 2025

Or, significantly increase the starting guess period for the second orbit.

@JonasKoziorek
Copy link
Contributor Author

JonasKoziorek commented Jan 10, 2025

I just used two identical initial guesses. If I could simulate the CI environment somehow locally, I could perhaps debug why the CI fails but waiting 10 mins every time I make a change is tedious.

@Datseris
Copy link
Member

But that's a poor choice no? We want to test that the algorithm can find at least two different periodic orbits, as it should. I thought that's what the test was trying to do (in the second half)

@Datseris
Copy link
Member

Does it not work with a big change like 10 units of time?

@JonasKoziorek
Copy link
Contributor Author

JonasKoziorek commented Jan 13, 2025

But that's a poor choice no? We want to test that the algorithm can find at least two different periodic orbits, as it should. I thought that's what the test was trying to do (in the second half)

Yes I know that it is a poor choice.
Just to be clear, for this algorithm the orbit is either found or not. So minimum 0 maximum 2 orbits. Ideally, I would like each initial guess to give me a different orbit.

@JonasKoziorek
Copy link
Contributor Author

Does it not work with a big change like 10 units of time?

I can try, but why +10? Does Lorenz System have such orbits? The guess of time acts the same as a guess of spatial coordinates. If there is no orbit near with the spatial guess + temporal guess, the algorithm won't converge.

@Datseris
Copy link
Member

I am confused: I thoguth you where searching for unstable periodic orbits in the chaotic lorenz system?

@JonasKoziorek
Copy link
Contributor Author

JonasKoziorek commented Jan 14, 2025

I am confused: I thoguth you where searching for unstable periodic orbits in the chaotic lorenz system?

Yes, I know there should be infinitely many of them. But nevertheless, this algorithm doesn't work so well that it would converge to any guess I input. I think the guess has to be quite precise for the algorithm to converge.

@Datseris
Copy link
Member

Okay, then let's simply remove the broken test and make a statement in the algorithm doc that the algorithm is not particularly accurate.

@Datseris Datseris merged commit b148ec1 into main Jan 15, 2025
2 checks passed
@Datseris Datseris deleted the fix_docs_build branch January 15, 2025 13:22
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