Skip to content

Conversation

@matbesancon
Copy link
Member

The hash functions were inconsistent with the equality tests, resulting in subtle failures, for instance in unique(graphs).

I used a hash close to the == function defined next to each of the simple graph types

@matbesancon matbesancon added the bug Something isn't working label Jan 23, 2026
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jan 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.19%. Comparing base (1d94b7a) to head (e1cab70).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #485   +/-   ##
=======================================
  Coverage   97.19%   97.19%           
=======================================
  Files         121      121           
  Lines        7092     7105   +13     
=======================================
+ Hits         6893     6906   +13     
  Misses        199      199           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nsajko
Copy link
Member

nsajko commented Jan 23, 2026

Rough idea for alternative implementation approach: uniquely represent the graph with matrices (a SimpleGraph with an adjacency matrix, a SimpleDiGraph with a weights matrix and an adjacency matrix). Then it's possible to rely on the hash(::AbstractArray, ::UInt) implementation that comes with Julia.

@matbesancon
Copy link
Member Author

I don't think converting to matrices is a good idea, hash may be called a lot for some operations and building adjacency matrices allocates a ton

@matbesancon
Copy link
Member Author

I removed the xor in the last version. After checking other projects, the UInt is the hardcoded assumption everywhere so we should probably keep it

@nsajko
Copy link
Member

nsajko commented Jan 23, 2026

I don't think converting to matrices is a good idea, hash may be called a lot for some operations and building adjacency matrices allocates a ton

The idea is to have an AbstractMatrix that's not a dense representation. However this bugfix PR should probably be merged ASAP in any case.

@matbesancon
Copy link
Member Author

even allocating a sparse matrix is not a no-op, it does allocate, I don't think it should be the case for hash when it can be avoided

@Krastanov Krastanov changed the title simple graph hash implementing a simple graph hash to correct for a buggy hash / == inconsistency Jan 24, 2026
@Krastanov
Copy link
Member

I modified the PR title to be a bit more descriptive because currently we do not have a changelog file and the PR title is the only thing that will show up in the autogenerated github release notes.

I also bumped the version so that we can make an immediate release?

@nsajko , do you consent to my suggestion to keep the code simple at the expense of a non-allocating typestable slowdown in <1.12? I completely agree with the factual technical statements you are making, I just want to have an evaluation how strongly you feel about the more-subjective judgement call of whether this is a bad policy.

@nsajko
Copy link
Member

nsajko commented Jan 24, 2026

I consent. I don't feel strongly about Julia LTS performance.

@matbesancon
Copy link
Member Author

great, thanks for the feedback

@matbesancon
Copy link
Member Author

I'll let someone else merge it to avoid merging my own PR

@nsajko nsajko merged commit a716a84 into master Jan 25, 2026
11 of 14 checks passed
@nsajko nsajko deleted the hash-simplegraphs branch January 25, 2026 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants