Add UniQ partitioning algorithm #16
Conversation
| return sub_circs | ||
|
|
||
|
|
||
| class dptask: |
There was a problem hiding this comment.
naming convention please follow google style guide
| self.gateNum = gateNum | ||
| self.qubitNum = qubitNum | ||
|
|
||
| def addGate(self, gateIndex: int, gatePos): |
There was a problem hiding this comment.
Similarly, please refer to the style guide.
In short
- For classes, use CamelCase convention.
- For methods, variables, use
lower_case_with_underscoreconvention.
|
|
||
|
|
||
| class UniQPartitioner(BasePartitioner): | ||
| """Partitioner in UniQ""" |
There was a problem hiding this comment.
Suggest to add a reference here. Refer to sphinx docstring sections to see commonly used keywords.
Currently, we're not creating a document website. But trying to write standard docstrings will reduce lots of effort once we need to have a website holding documentation of this project.
"""
References:
[1] <the-paper-link>
"""
|
|
||
| def run(self, circuit: Any) -> List[QdaoCircuit]: | ||
| self._circ_helper.circ = circuit | ||
| ops = copy.deepcopy(self._circ_helper.instructions) |
There was a problem hiding this comment.
why do we need to make a deep copy of instructions? If necessary, we need to measure the performance overhead when circuit becomes very large with a large number of instructions.
There was a problem hiding this comment.
Because after each selection of a subcircuit, the selected gate must be removed and preprocessed again. In order not to affect the original data, a deep copy operation is selected. This really isn't necessary and if it would cause a performance hit, I would build an array to record this kind of property.
There was a problem hiding this comment.
based on my understanding, all operations you performed on ops is pop() and get(). As long as we do not need to modify, i.e., write to, the value of ops[i]. There's no need to use deepcopy.
| return (opList, numQubit) | ||
|
|
||
|
|
||
| class UniQPartitioner(BasePartitioner): |
There was a problem hiding this comment.
Maybe we should consider registering UniQPartitioner into PARTITIONERS. This is somehow a naive factory design pattern offering better maintainablility. We could discuss more elegant practice.
| set(self.result_op[gateIndex + 1][j]) | ||
| ) | ||
|
|
||
| def createTask(self, ops): |
There was a problem hiding this comment.
This function name will confuse users and developers. Basically, we already defined DpTask class, and one might think that DpTask() will create a task object. So I suggest to consider better naming to distinguish this function from vanilla DpTask instantiation.
| @@ -0,0 +1,48 @@ | |||
| from qiskit import transpile | |||
There was a problem hiding this comment.
Maybe we do not need to commit this file into this project. If necessary, it would be better to appear in our test directory
| print("----------UniqPartitioner-----------") | ||
| print("num of sub-circuits:" + str(len(sub_circs))) |
There was a problem hiding this comment.
Maybe it would be better to use logging, if this is just used for testing, it is not necessary to commit these lines.
| active = self._np | ||
| m = self._circ_helper.circ.num | ||
| sub_circs = [] | ||
| while len(ops) != 0: |
| bitList = self.result_bit[i + 1][j] | ||
| numQubit = len(self.result_bit[i + 1][j]) | ||
| numOp = len(self.result_op[i + 1][j]) | ||
| return (opList, numQubit) |
There was a problem hiding this comment.
not necessary to explicitly use tuple. return op_list, num_qubits would be enough
|
Firstly, we need to fix unit test, it seems that current implementation is not compatible with python3.8. |
|
There are indeed many problems with the code, thank you for your feedback. |
No description provided.