Fix pylint mypy issues - #413
Conversation
| def outputs(self) -> Iterator[LogicalCircuitNode]: | ||
| return (cast(LogicalCircuitNode, node) for node in super().outputs) | ||
| def outputs(self) -> Sequence[LogicalCircuitNode]: | ||
| return super().outputs |
There was a problem hiding this comment.
inputs and outputs are already defined in super class Graph and DiAcyclicGraph in algorithms.py, and outputs should be a Sequence, not Iterator, similar to lines 142-148 in here
| in_nodes = self._in_nodes | ||
| in_nodes: dict[LogicalCircuitNode, list[LogicalCircuitNode]] = { | ||
| node: list(inputs) for node, inputs in self.nodes_inputs.items() | ||
| } |
There was a problem hiding this comment.
original in_nodes has type Mapping[LogicalCircuitNode, Sequence[LogicalCircuitNode]], but this function has dict and list operations (extend, append…) on it
There was a problem hiding this comment.
Do we ever use an immutable mapping or have a need for one ? Maybe we can just switch Mapping to MutableMapping in the parent class ? @loreloc
There was a problem hiding this comment.
I don't think we ever use immutable mapping, but I failed to see how this is related to immutable mapping?
The new in_nodes creates a mutable local copy with type dict[LogicalCircuitNode, list[LogicalCircuitNode]] instead of the original type Mapping[LogicalCircuitNode, Sequence[LogicalCircuitNode]] in the parent
So the parent is generic and the local is specific. (If we use MutableMapping in the parent class, we will also need MutableSequence in the parent class, so MutableMapping[NodeT, MutableSequence[NodeT]] , too restrictive?)
Fix these
cirkit/templates/logic/graph.py:209: error: Unsupported target for indexed assignment ("Mapping[LogicalCircuitNode, Sequence[LogicalCircuitNode]]") [index]
cirkit/templates/logic/graph.py:220: error: "Sequence[LogicalCircuitNode]" has no attribute "extend" [attr-defined]
cirkit/templates/logic/graph.py:223: error: Unsupported target for indexed assignment ("Mapping[LogicalCircuitNode, Sequence[LogicalCircuitNode]]") [index]
cirkit/templates/logic/graph.py:224: error: "Sequence[LogicalCircuitNode]" has no attribute "append" [attr-defined]
cirkit/templates/logic/graph.py:227: error: "Sequence[LogicalCircuitNode]" has no attribute "remove" [attr-defined]
cirkit/templates/logic/graph.py:229: error: "Sequence[LogicalCircuitNode]" has no attribute "insert" [attr-defined]
| pic_net = self # Capture reference to self | ||
|
|
||
| def forward_hook(module, input, output): | ||
| def forward_hook(_module, _input, _output): |
There was a problem hiding this comment.
Fix these
cirkit/backend/torch/parameters/pic.py:262:25: W0613: Unused argument 'module' (unused-argument) cirkit/backend/torch/parameters/pic.py:262:33: W0613: Unused argument 'input' (unused-argument) cirkit/backend/torch/parameters/pic.py:262:40: W0613: Unused argument 'output' (unused-argument)
IrwinChay
changed the title
chbricout
left a comment
chbricout
left a comment
There was a problem hiding this comment.
Good PR! I just have this one question regarding MutableMapping, not sure if we intended to have an immutable structure as the code is not necessarily oriented towards that (in place operations and all).
I would like to have a quick opinion from Lorenzo and/or @arranger1044
2 participants
This PR fixes all the existing linting and typing issues outputted by pylint and mypy.
Currently mypy found no issues, and all issues found by pylint are of missing class or function docstrings, since some files (e.g.
cirkit/backend/torch/rules/parameters.py) have no docstrings at allThe reasonings for some changes are shown below (in the conversation)
I think we should write the missing docstrings in another PR, which might not be in near future and might requires further discussions (since it affects the documentation). Noted down in Issue #414