1.0.0 alpha #434
Merged
1.0.0 alpha #434
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
peterrrock2
force-pushed
the
1.0.0-alpha
branch
from
c6cf4de to
62443d2
Compare
I added a new Graph object in graph.py that is NOT a subclass of NetworkX but instead has two internal data members, nxgraph and rxgraph. The new Graph object supports most (almost all) of the syntax of the old Graph object, which should allow us to add RX support seamlessly in the near future...
a Partition. In particular, support for translating node_ids from subgraphs to parent graph node_ids. Total PITA... Lots of cleanup needed and also lots of tests, but the regression test works again - tests/frm_tests/test_frm_regression.py
… the Graph class. Also updated a bunch of tests to work with the new RX based code.
as is in: NxGraph() => is_nx_graph()
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...
* 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
* Some changes are cosmetic * Some are results of resolving a TODO: item All tests pass...
No new functionality in GerryChain...
with a subtag that said whether the TODO: was for Performance, Documentation, Refactoring (basically any stylistic or software engineering improvement that did not affect functionality), and Code (which was a catchall for stuff that should be done sooner rather than later for one reason or another). This commit is just to allow a reviewer to be able to ignore these changes as trivial.
updated function signature comments, lots of comments more generally, updated TODO: comments (actually "did" a lot of them)
peterrrock2
force-pushed
the
1.0.0-alpha
branch
from
62443d2 to
1ceb7ed
Compare
cdonnay
suggested changes
Dec 12, 2025
Contributor
cdonnay
left a comment
cdonnay
left a comment
There was a problem hiding this comment.
Just one small change to our org name.
How much of these refactoring comments do you want to keep for now?
Collaborator
Author
|
Let's keep all the comments in there until we are ready for full release and then we will clean things up |
Co-authored-by: Chris Donnay <[email protected]>
cdonnay
approved these changes
Dec 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Start the process of migrating to RustworkX internally. This will be a prerelease so that we can do some user testing