fix wasm issues #1691
fix wasm issues #1691
Conversation
cqc-melf
force-pushed
the
melf/fix-wasm-issue
branch
from
8879c3e to
db9cdd6
Compare
tket/test/src/test_wasm.cpp
Outdated
There was a problem hiding this comment.
All the appending tests here involve appending to an empty circuit. Could we add some tests appending to a circuit that already contains WASM wires?
There was a problem hiding this comment.
I have added one, see d9e6dac
There is also one in the python tests already.
| @@ -413,6 +413,9 @@ void def_circuit(py::class_<Circuit, std::shared_ptr<Circuit>> &pyCircuit) { | |||
| .def_property_readonly( | |||
| "n_gates", &Circuit::n_gates, | |||
| ":return: the number of gates in the Circuit") | |||
| .def_property_readonly( | |||
| "wasm_uid", &Circuit::get_wasm_file_uid, | |||
| ":return: the wasm uid of the first wasmop found in the circuit") | |||
There was a problem hiding this comment.
Can a circuit contain more than one WASM UID? If so it would be preferable to return the full set here. If not, we can be explicit about this in the doc.
There was a problem hiding this comment.
It should only be one
cqc-melf
force-pushed
the
melf/fix-wasm-issue
branch
from
c035e24 to
ac1eb5a
Compare
cqc-melf
force-pushed
the
melf/fix-wasm-issue
branch
from
ac1eb5a to
14b5146
Compare
pytket/tests/classical_test.py
Outdated
|
|
||
| assert c.depth() == 0 | ||
| with pytest.raises(ValueError): | ||
| c.wasm_uid |
There was a problem hiding this comment.
I think it would be a bit nicer if this returned None when there is no WASM wire, rather than throw an exception.
|
@cqc-alec Is it expected that some changes to the stubs do not make the check fail? As you can see in the last commit, there have been some changes, that did not show up on the CI? |
| std::optional<std::string> result; | ||
| try { | ||
| result = circ.get_wasm_file_uid(); | ||
| } catch (const std::exception &) { |
There was a problem hiding this comment.
Can we propagate the change to the C++, i.e. male get_wasm_file_uid() return a std::optional<std::string>? I think that's better than a catch-all exception handler, and could actually make the C++ code nicer too.
pytket/tests/zx_diagram_test.py
Outdated
| @@ -12,7 +12,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| from math import isclose, pow | |||
| from math import isclose, pow # noqa: A004 | |||
There was a problem hiding this comment.
I suggest just removing the pow import and using the built-in one; as used in this file there is no difference.
There was a problem hiding this comment.
done in the merge commit
Not sure what you mean here. In the last commit both the binder code and the stubs changed, so I would not expect the check fail? |
| """ | ||
| :return: the wasm uid of the first wasmop found in the circuit | ||
| :return: the unique wasm uid of the circuit |
There was a problem hiding this comment.
@cqc-alec this line was already changed in the other commit, but the stubs check has not shown this difference.
There was a problem hiding this comment.
Certainly it should fail if they don't match, but I'm struggling to see where there is/was a mismatch.
There was a problem hiding this comment.
The comment was changed in 14b5146, but the CI check has not show this difference.
pytket/docs/changelog.rst
Outdated
| @@ -1,13 +1,22 @@ | |||
| Changelog | |||
| ========= | |||
|
|
|||
| Unreleased | |||
| ---------- | |||
| 1.36.0 (unreleased) | |||
There was a problem hiding this comment.
Prefer to keep this as "Unreleased" only. We don't know if it will be 1.36.0 or 1.35.1 ...
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
cqc-melf
force-pushed
the
melf/fix-wasm-issue
branch
from
4c2a42f to
218ccd5
Compare
Description
Fixes:
#1641
#1504
#1546
Checklist