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

add topology enums and dispatch decorator #248

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

boothby
Copy link
Contributor

@boothby boothby commented Dec 25, 2024

This is a preliminary patch to address #210 -- we add a topology enum to dnx.topology, and a decorator topology_dispatch.

The topology enum is pretty simple, except that I want the machinery of py3.11's StrEnum (that is, CHIMERA == 'chimera'), so I fair-use'd barebones implementations of StrEnum and global_enum from the CPython3.11 library.

The topology_dispatch decorator can be used to define a generic function in one file, and then in other files, define specializations based on the topology family. For example, I've added a defect_free generic function in dnx.generators.common and then added specializations for each of CHIMERA, PEGASUS, ZEPHYR. Thus, to get a defect-free version of any qubit topology, one can simply call dnx.generators.common.defect_free(G).

#############
# common.py
#
@topology_dispatch
def defect_free(G):
    """Construct a defect-free topology based on the properties of G."""
    raise NotImplementedError(f"no defect-free generator defined for {G.graph.get('family')} graphs")


#############
# chimera.py
#
from ..topology import CHIMERA
from .common import defect_free

@defect_free.install_dispatch(CHIMERA)
def defect_free_chimera(G):
    """Construct a defect-free Chimera graph based on the properties of G."""
    attrib = G.graph
    family = attrib.get('family')
    if family != CHIMERA:
        raise ValueError("G must be constructed by dwave_networkx.chimera_graph")
    args = attrib['rows'], attrib['columns'], attrib['tile']
    kwargs = {'coordinates': attrib['labels'] == 'coordinate'}
    return chimera_graph(*args, **kwargs)

@boothby boothby marked this pull request as draft December 25, 2024 00:34
Copy link

codecov bot commented Dec 25, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 12 lines in your changes missing coverage. Please review.

Project coverage is 76.68%. Comparing base (7d8aa45) to head (e8bd004).

Files with missing lines Patch % Lines
dwave_networkx/drawing/pegasus_layout.py 66.66% 3 Missing ⚠️
dwave_networkx/drawing/chimera_layout.py 75.00% 2 Missing ⚠️
dwave_networkx/drawing/zephyr_layout.py 75.00% 2 Missing ⚠️
dwave_networkx/topology.py 93.75% 2 Missing ⚠️
dwave_networkx/generators/chimera.py 94.11% 1 Missing ⚠️
dwave_networkx/generators/pegasus.py 95.23% 1 Missing ⚠️
dwave_networkx/generators/zephyr.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #248      +/-   ##
==========================================
+ Coverage   75.05%   76.68%   +1.62%     
==========================================
  Files          31       32       +1     
  Lines        2213     2290      +77     
==========================================
+ Hits         1661     1756      +95     
+ Misses        552      534      -18     

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

@boothby
Copy link
Contributor Author

boothby commented Dec 31, 2024

After some thought, I wasn't a fan of my first approach; it was a bit too kludgy. Re-reading the comments of dwavesystems/minorminer#210, @pau557 had given me a better suggestion for notation which I hadn't followed in the above. My revised approach collects generic methods into the enumeration members. Here, I avoid circular imports through the use of an ImplementationHook which is used to monkeypatch itself away during the initial import.

With this new approach, I don't need to add generic functions to multiple files; instead, I collect their names into topology.py. Instead of that defect_free function, I instead make a defect_free_graph slot for the Topology enum members, and when I implement defect_free_chimera, it looks like:

@CHIMERA.defect_free_graph.implementation
def defect_free_chimera(G):
    ...

After making this change, my choice to use StrEnum had a negative impact: the namespace was polluted with the various methods of str. On reflection, I think that it's important to preserve a few features of str since I'm populating the dwave_networkx graph "family" property with these enum members -- namely, __eq__, __hash__, and __str__ should be enough for backwards compatibility and future convenience.

@boothby boothby marked this pull request as ready for review December 31, 2024 23:07
Copy link
Member

@arcondello arcondello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a pass for code. Needs to ruminate on the design a bit. Will try to do another pass later today or tomorrow.

dwave_networkx/drawing/chimera_layout.py Outdated Show resolved Hide resolved
dwave_networkx/topology.py Outdated Show resolved Hide resolved
dwave_networkx/topology.py Outdated Show resolved Hide resolved
dwave_networkx/topology.py Outdated Show resolved Hide resolved
tests/common.py Outdated Show resolved Hide resolved
@@ -71,7 +72,7 @@ def chimera_layout(G, scale=1., center=None, dim=2):

# now we get chimera coordinates for the translation
# first, check if we made it
if G.graph.get("family") == "chimera":
if G.graph.get("family") == CHIMERA:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be compared by identity since both are enum members, right?

Suggested change
if G.graph.get("family") == CHIMERA:
if G.graph.get("family") is CHIMERA:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern with making that change is backwards-compatibilty with pickled graphs. I'm happy to make the change if you're unconcerned with that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly concerned, but I can't say that I'm aware of how much (if at all) this might disrupt some users that have old pickled instances of graphs. If so, keeping it as an eq comparison and adding a comment about it being kept for backward compatibility (potentially a deprecation note for the future).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should preferably be tracked in a changelog, possibly under breaking.

"""Construct a defect-free Chimera graph based on the properties of G."""
attrib = G.graph
family = attrib.get('family')
if family != CHIMERA:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if family != CHIMERA:
if family is not CHIMERA:

Similarly in other files.

Comment on lines +258 to +259
def defect_free_chimera(G):
"""Construct a defect-free Chimera graph based on the properties of G."""
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Parameters and Returns.

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