bifurcationkit / BifurcationKit.jl

A Julia package to perform Bifurcation Analysis
https://bifurcationkit.github.io/BifurcationKitDocs.jl/stable
Other
303 stars 37 forks source link

Log branching errors in `bifurcationdiagram` #134

Closed danielwe closed 10 months ago

danielwe commented 10 months ago

bifurcationdiagram silently swallows all exceptions during branching. This kept me in the dark about a simple method ambiguity error when trying to use a custom eigsolver---I couldn't understand why I got an amputated diagram but didn't see any warnings or errors.

So here's a proposed improvement: log every exception and include the same info that's printed when verbosity is turned on. Errors are still non-fatal, but at least the user will know about them.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (72fff43) 89.70% compared to head (962761b) 89.69%.

Files Patch % Lines
src/bifdiagram/BifurcationDiagram.jl 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #134 +/- ## ========================================== - Coverage 89.70% 89.69% -0.02% ========================================== Files 60 60 Lines 8498 8498 ========================================== - Hits 7623 7622 -1 - Misses 875 876 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rveltz commented 10 months ago

I dont understand why you need to introduce the function branchinfo (see simple code below where I put back @error ex. This would error and I doubt the line

println("─"^80*"\n──▶ New branch, level = $(level+1), dim(Kernel) = ", kernel_dimension(pt), ", code = $code, from bp #",id," at p = ", pt.param, ", type = ", type(pt))

is erroring so it would output the println. Is it?

try
    verbose && println("─"^80*"\n──▶ New branch, level = $(level+1), dim(Kernel) = ", kernel_dimension(pt), ", code = $code, from bp #",id," at p = ", pt.param, ", type = ", type(pt))
    γ = letsbranch(id, pt, level)
    add!(node, γ, level+1, id)
    ~isnothing(γ) && (verbose && printstyled(color = :green, "────▶ From ", type(from(γ)), "\n"))
catch ex
     @error ex
end
danielwe commented 10 months ago

Just a suggestion---my idea was to make sure this information is available locally in the log message, since log messages and println don't always go to the same stream, so otherwise it might not be obvious which branch errored. I'll use Pluto.jl to demonstrate this point since that's the environment I'm working in right now, but custom log message handling is used in many other contexts as well:

image

Note how the log message cards are separate from the console output.

Of course, an alternative solution would be to replace the verbose && println(...) idiom with @debug log messages throughout the package. That would ensure that these messages and @error are handled in the correct order by the same backend. But it would be a much bigger and breaking change with unclear overall benefits. Seems like a discussion for another day.

Anyway, your point is well taken. In most cases, a simple @error exception=ex and using verbose=true will provide all the information needed for debugging, and it's a simpler and more local change. I'll update the PR.

danielwe commented 10 months ago

How about this version? I kept a short message there just to give some minimal context around the error log.

rveltz commented 10 months ago

Thanks a lot for correcting this