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

Add LineSearchTestCase #177

Merged
merged 4 commits into from
Aug 5, 2024
Merged

Conversation

timholy
Copy link
Contributor

@timholy timholy commented Jul 29, 2024

Also includes the failing case in PR#174.

This is minimally modified from the code provided by @mateuszbaran (who I've listed as a co-author).

One thing that seems missing: documentation on how to capture the specific values tried by the failing line search method. I'm not broadly familiar with them all, is there any good guidance on that? Or should we add a cache::Bool option to them all?

Once this gets merged, we should first capture any other problem reports and then start trying to fix the failures.

Closes #102

Also includes the failing case in PR#174.

Co-authored-by: Mateusz Baran <[email protected]>
@mateuszbaran
Copy link
Contributor

Thanks for continuing your work on this issue 👍 . I would generally hope that line search failures would be infrequent enough that convenience options for saving values used for interpolation wouldn't be necessary? After this PR it would be enough to ]dev LineSearches, add println instructions to the line search and copy the three vectors to LineSearchTestCase. It's still much less work than figuring out that the line search might be the issue, so probably documenting this workflow would be sufficient?

@timholy
Copy link
Contributor Author

timholy commented Jul 30, 2024

I've also found a weird NaN bug with MoreThuente (might be in my code, might not), so it may be a broader issue? And MoreThuente doesn't seem to cache the values so it may be less straightforward to capture.

@timholy
Copy link
Contributor Author

timholy commented Jul 30, 2024

OK, I think this is ready to merge. I don't have privileges myself so I'll rely on @pkofod to review and make final decisions.

@timholy
Copy link
Contributor Author

timholy commented Jul 30, 2024

Note this also adds a new supertype, AbstractLineSearch, for all methods. I can delete this if you don't like it, but it doesn't seem like a bad thing.

@pkofod
Copy link
Member

pkofod commented Aug 5, 2024

Thanks! I'll review

@pkofod pkofod merged commit 3259cd2 into JuliaNLSolvers:master Aug 5, 2024
15 checks passed
@timholy timholy deleted the teh/testcase branch August 5, 2024 14:36
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.

Add logging functionality
3 participants