Topology utils#275
Conversation
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 |
There was a problem hiding this comment.
My rule throughout:
Type mismatch -> NotImplemented (defer to the other operand).
Same-type -> always return a real bool.
Comparing two same-type objects that happen to differ in shape/quotient-ness/etc. isn't "uncomparable," it's just unequal, so that returns False, not a defer. There were a couple of bugs before in this case, are all now fixed in 0717379
|
|
||
| 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 |
There was a problem hiding this comment.
I'd rather leave this as NotImplemented and not lock in False. For now, I'm not ready to commit to that yet; might want different coordinates of the same node to be declared as equal, or perhaps a subclass to define their own equality down the line. No cost to keeping it open. Happy to change it once we've actually decided what equality means for these.
There was a problem hiding this comment.
My rule throughout:
Type mismatch -> NotImplemented (defer to the other operand).
Same-type -> always return a real bool.
Comparing two same-type objects that happen to differ in shape/quotient-ness/etc. isn't "uncomparable," it's just unequal, so that returns False, not a defer. There were a couple of bugs before in this case, are all now fixed in 0717379
|
|
||
| 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?
There was a problem hiding this comment.
My rule throughout:
Type mismatch -> NotImplemented (defer to the other operand).
Same-type -> always return a real bool.
Comparing two same-type objects that happen to differ in shape/quotient-ness/etc. isn't "uncomparable," it's just unequal, so that returns False, not a defer. There were a couple of bugs before in this case (including in this method), are all now fixed in 0717379
Co-authored-by: Radomir Stevanovic <radomir.stevanovic@gmail.com> Co-authored-by: Theodor Isacsson <tisacsson@dwavesys.com>
4c018d7 to
b7c7fb4
Compare
…ClassVar attributes Implement _set_edge for Edge, fix typing hints for args add document Make TopologyNode a properly-enforced ABC Fix indentation Fix comparison operands Make NeighborContributorMixin an abstractclass and change some subclasses' methods into abstractmethods
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.