Changes to get all tests to pass - some cleanup as well #431
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
August 16, 2025 11:18
the tests to pass (as well as writing tests for new code added to
graph.py). There is a little bit of general cleanup as well.
A good bit of the changes were just to deal with NX node_ids in tests
that required translation to or from RX node_ids or to / from what
I refer to as "original" node_ids which are the NX node_ids given
to the original NX graph.
I had to essentially duplicate the tests in test_tree.py to test
RX-based Graph objects in addition to NX-based Graph objects.
I made an executive decision (which you can un-decide) concerning
Election objects. The original code allowed creation of an
Election object with an explicit map of {node_id: vote_tally_integer}.
This was then used to create the initial DataTally objects, which
was problematic because the node_ids were NX node_ids which were
not appropriate for tallying data in the RX-based graph that
was in the partition. So, I just added code to the Election
class prohibiting the use of node_ids - instead you can only
use attribute names. I checked and there was no other use
of explicit node_id to vote count data in any of the tests or
in any of the code examples you sent me (note that I did not
look at the User Guide or any other example notebook code).
I then changed the one test case to use attribute names...
peterrrock2
reviewed
Oct 21, 2025
Collaborator
peterrrock2
left a comment
peterrrock2
left a comment
There was a problem hiding this comment.
These are looking great so far! Just poke @cdonnay once you are ready for merge!
* These changes were made based on a comparison of NX vs RX.
I modified the code to allow running with either an NX-based
partition graph or an RX-based partition graph. This allowed
for an apples to apples comparison of the differences
between NX and RX. The biggest gain was from using
RX inside implementation of random_spanning_tree. The
changes below resulted in the current code eventually
running in half the time when running on RX vs. NX.
* Summary of changes:
* Turned verify_graph_is_valid to a noop
=> This had no effect on NX vs. RX, but it did speed things up
* Changed order of checking the kind of embedded graph object to
first check for RX, since that will be the case most of the
time.
=> Lots of places where this was done...
* Use RX functions to access edges and edge data
* Use get_edge_indices_by_index() instead of getting the entire
list of edges and then selecting the desired one.
* Use edge_indices_from_endpoints() instead of getting the
entire list of edges and comparing nodes...
* Use get_edge_data_by_index() instead of selecting from
list of all edges.
* Changed neighbors() to NOT convert RX.neighbors() to a list since
the returned object (NodeIndices) already has list-like behavior
Author
|
I have either changed the code as suggested or I have inserted a TODO: comment in the code if I still have some head scratching to do... -Fred |
Contributor
|
Hi @chief-dweeb ! First of all, love your user name.
|
Contributor
|
Either way, I think this is definitely at a good spot to merge. And Peter will be back soon to talk next steps. Cannot tell you how much we appreciate all the work you do @chief-dweeb !! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary mission was to get all of the tests to pass, and now all of the tests do indeed pass.
There is also some cleanup code - deleting old comments, changing variable names, etc.
There was one interesting issue in test_updaters.py that is documented in the changes to the code. I changed the semantics a tiny bit to avoid NX "original" node_ids being used for Elections because the changes to allow it would have been a lot of complexity for no significant gain - but perhaps you will disagree...