Topology utils#275
Conversation
Fix import Fix syntax
d1303ed to
1da2b4c
Compare
|
@thisac Is this ready to be merged? |
randomir
left a comment
randomir
left a comment
There was a problem hiding this comment.
Did a quick review, it looks good, but I would like to do an in-depth review before me merge this.
The suggestions below are mostly minor, except the use of function objects as default argument values.
|
|
||
| def __eq__(self, other: object) -> bool: | ||
| if not type(self) is type(other): | ||
| return NotImplemented |
There was a problem hiding this comment.
Why not:
| return NotImplemented | |
| return False |
| def neighbors( | ||
| self, | ||
| nbr_kind: EdgeKind | Iterable[EdgeKind] | None = None, | ||
| where: Callable[[Coord], bool] = lambda coord: True, |
There was a problem hiding this comment.
I believe the mutable default argument value gotcha applies to function objects as well.
| associated_edgekind: EdgeKind | ||
|
|
||
|
|
||
| class HasInternalNeighborsMixin(NeighborContributorMixin): |
| Returns: | ||
| TopologyShape: The shape converted to its corresponding quotient shape. | ||
| """ | ||
| @abstractmethod |
There was a problem hiding this comment.
| @abstractmethod | |
| @abstractmethod | |
| @@ -0,0 +1,20 @@ | |||
| # Copyright 2016 D-Wave Systems Inc. | |||
There was a problem hiding this comment.
Since it's a new file.
| # Copyright 2016 D-Wave Systems Inc. | |
| # Copyright 2026 D-Wave |
| def __init__(self, *args, **kwargs) -> None: | ||
| super().__init__(*args, **kwargs) |
There was a problem hiding this comment.
Should be moved to the top of the class.
| @classmethod | ||
| def topology_name(cls) -> str: | ||
| """Returns the name of the topology associated with the class. | ||
|
|
||
| Returns: | ||
| str: The name of the topology the class is designed for. | ||
| """ | ||
| if hasattr(cls, "_topology_name"): | ||
| return cls._topology_name | ||
| raise NotImplementedError | ||
|
|
||
| @classmethod | ||
| @abstractmethod | ||
| def kind(cls) -> CoordKind: | ||
| """Returns the kind of the coordinate associated with the class. | ||
|
|
||
| Returns: | ||
| CoordKind: The kind of class's coordinate. | ||
| """ |
There was a problem hiding this comment.
These two both are used to return a static property of the different coord classes, but are implemented in different ways. Also, these should probably be properties instead, so that you call them as ZephyrCoord.topology_name.
The simplest I think might be to only have them declared directly in the subclasses, and skip them in the baseclass altogether (or have a topology_name = None).
class ZephyrCoord(Coord):
topology_name: str = "zephyr"There was a problem hiding this comment.
A more interesting solution might be to create a metaclass for the property, which should act like a property for the subclasses themselves rather than for instances of them.
class _CoordClassProperties(type):
@property
def topology_name(cls) -> str:
return cls._topology_name
class Coord(metaclass=_CoordClassProperties):
...
class ZephyrCoord(Coord):
_topology_name: str = "zephyr" But this might be a bit overkill.
|
|
||
| def __eq__(self, other: object) -> bool: | ||
| if (type(self) is not type(other)) or (self.is_quotient() != other.is_quotient()): | ||
| return NotImplemented |
There was a problem hiding this comment.
Seems like returning False might make more sense, no?
| return NotImplemented | |
| return False |
|
|
||
| def __eq__(self, other: object) -> bool: | ||
| if not (type(self) is type(other) and self._shape == other._shape): | ||
| return NotImplemented |
There was a problem hiding this comment.
Similarly to in other places, why not return False?
| return f"{coord.to_tuple(), self._shape.to_tuple()}" | ||
|
|
||
|
|
||
| class NeighborContributorMixin: |
There was a problem hiding this comment.
This could potentially also be made abstract, as well as turning all the NotImplementedError methods in subclasses below into abstractmethods.
| except ValueError: | ||
| continue |
There was a problem hiding this comment.
When are these value errors expected? Is there a risk that there might be errors that should be raised being bypassed silently? Similarly for other generator methods below.
| ) -> nx.Graph: | ||
| return zephyr_graph(*shape, create_using, node_list, edge_list, data, coordinates, |
Co-authored-by: Radomir Stevanovic <radomir.stevanovic@gmail.com> Co-authored-by: Theodor Isacsson <tisacsson@dwavesys.com>
3 participants
The added files in
dwave/graphs/topologies/commonprovide blueprint classes for working with different aspects of any D-Wave topology.coord.py: Coordinate systems used for node labels.node_edge.py: Nodes and edges.planeshift.py: Displacement of nodes in the Cartesian plane.shape.py: Shape of a graph.topology.py: Constructing a topology graph whose nodes and edges are equipped with all the specific-topology-relevant functionalities.The added files in
dwave/graphs/topologies/zephyr, i.e.zcoord.py,znode_edge.py,zplaneshift.pyandzshape.py, contain the implementation of the corresponding objects for Zephyr topology. Inzephyr.pythe classZephyrhas been added that is built onTopologyblueprint indwave/graphs/topologies/common/topology.pyThe test suite for these modules is added in
tests.Note: This is an evolution of previous
zephyr_utilsPR tominorminer.