Optimize node_swap#735
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
leotrs
left a comment
There was a problem hiding this comment.
Nice cleanup Max. The dict-mapping comprehension is much easier to read than the three-pass temp-ID swap, the deepcopy goes away, and the new tests cover the previously implicit edge cases (self-inverse on both-in-one-edge, original-not-mutated, attribute preservation).
One thing worth flagging: removing the id_temp parameter is a backward-incompatible change. Anyone passing node_swap(H, a, b, id_temp=...) will get a TypeError. The arg was always implementation-detail leakage so dropping it is the right call, but worth mentioning in the changelog so it's an intentional v1.0 break rather than a silent one.
Optimize
node_swapby replacing the three-loop temp-ID swap with a single-pass dictionary mapping. Closes #642Old approach: used a temporary node ID to swap in three sequential passes — replace
nid1 → temp, thennid2 → nid1, thentemp → nid2— plus adeepcopyof the full edge dict.New approach: builds a
{nid1: nid2, nid2: nid1}mapping and remaps each edge's members in a single dict comprehension. Handles the edge case where both nodes appear in the same edge correctly (the swap is appliedatomically per member, leaving the set unchanged). Removes the
id_tempparameter and thedeepcopyimport entirely.Timing
Single swap on an
xgi.random_hypergraphaveraged over runs.The ~1.5x speedup is consistent across sizes.
Tests
Added some tests for uncovered cases.