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 floating point #3

Merged
merged 5 commits into from
Mar 22, 2024
Merged

fix floating point #3

merged 5 commits into from
Mar 22, 2024

Conversation

etiennedeg
Copy link
Member

@etiennedeg etiennedeg commented Oct 22, 2021

This is a repost of my PR on LightGraphsFlows.jl.
Closes #28.
is_zero is necessary because when comparing against zero, this is equivalent to strict equality (only absolute difference is tolerated). eps is not defined for Integers so we need to separate AbstractFloat from Integer.
From last PR, I re-coded the is_zero function to take advantage of multiple dispatch, and added a non regression test.

As we only add errors, should I use nv(g)*eps(T) instead of sqrt(eps(T))?

@jpfairbanks
Copy link
Member

Well nv(g)*eps could get pretty big so we probably want sqrt(eps). For example, if you use Int32 graphs with Float32 weights, then

julia> typemax(Int32) * eps(Float32)
256.0f0

Having is_zero(x) return true for x=255 seems bad.

@matbesancon does that reasoning sound right to you? If yes, then I'm ready to merge.

@etiennedeg
Copy link
Member Author

Is this reasonable to expect a graph with typemax(Int32) vertices ?
In fact, I think the error is more in the magnitude of the numbers added than in the number of addition (which should stay small at each vertex in the case of the push relabel). Maybe something like norm_capacity*eps(T) were the norm is calculated on the capacity matrix ? I'm really not an expert in error propagation, I don't remember from where I found the sqrt(eps(T)), but I think it comes from the analysis of multiplication and that may be overkill here.

@jpfairbanks
Copy link
Member

Yes, typemax(Int32) is only 2,147,483,647 a sparse graph on 2B vertices is ~100GB of memory. Big, but not an unreasonable target for Graphs.jl. I think we need to expose this tolerance with a keyword argument to the user. That way they can change it. sqrt(eps) is a good default, but we should expose that tolerance in the API.

@etiennedeg
Copy link
Member Author

@gdalle Can you check this is alright and integrate the PR in your move towards Graphs.jl?

src/maximum_flow.jl Outdated Show resolved Hide resolved
src/maximum_flow.jl Outdated Show resolved Hide resolved
src/maximum_flow.jl Outdated Show resolved Hide resolved
@gdalle gdalle merged commit 4adb081 into JuliaGraphs:master Mar 22, 2024
10 checks passed
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.

never ending max flow algorithm
3 participants