SimpleWeightedGraphs.jl icon indicating copy to clipboard operation
SimpleWeightedGraphs.jl copied to clipboard

Use `iszero(?)` instead of `?==zero(...)` for warning when adding edge, for Symbolics Num compatibility

Open kockahonza opened this issue 1 year ago • 7 comments

A partial fix/workaround for #49 with no downsides as far as I can tell. It is just a replacement of comparing to zero via == to using iszero. Though this isn't a comprehensive change to make all things work with Num types, it at least allows their creation (and they seem to mostly work in my usage).

kockahonza avatar Dec 09 '24 16:12 kockahonza

This seems fine to me, did you search for other uses of the pattern == zero( in the package while you're at it?

gdalle avatar Dec 09 '24 16:12 gdalle

I've done a really quick grep for "zero" and didn't see it used anywhere else. Thanks for the promptness!

kockahonza avatar Dec 09 '24 16:12 kockahonza

Test failures are unrelated and should be fixed by #51

gdalle avatar Dec 09 '24 17:12 gdalle

Would it make sense to add Symbolics to the tests and write a small unit test so that this will not be accidentally removed in future releases?

simonschoelly avatar Dec 09 '24 20:12 simonschoelly

I am very unfamiliar with Symbolics otherwise I would have tested it myself but is this also a case we have to consider? https://github.com/JuliaGraphs/SimpleWeightedGraphs.jl/blob/8128430c9f1a5a2f2cd9dd2b89d72916c811b257/src/overrides.jl#L71

simonschoelly avatar Dec 09 '24 20:12 simonschoelly

Honestly I don't think it's worth the addition, Symbolics is a huge and rather brittle dependency. But we can try to come up with a simpler, self-contained test.

gdalle avatar Dec 10 '24 05:12 gdalle

@kockahonza sorry for the delay! Can you merge the latest main to this branch and add a self-contained test for your changes?

gdalle avatar Jan 17 '25 12:01 gdalle