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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kockahonza
Copy link

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).

@gdalle
Copy link
Member

gdalle commented Dec 9, 2024

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

@kockahonza
Copy link
Author

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

@gdalle
Copy link
Member

gdalle commented Dec 9, 2024

Test failures are unrelated and should be fixed by #51

@simonschoelly
Copy link
Member

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
Copy link
Member

I am very unfamiliar with Symbolics otherwise I would have tested it myself but is this also a case we have to consider?

is_dangling = findall(S .== 0)

@gdalle
Copy link
Member

gdalle commented Dec 10, 2024

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.

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.

3 participants