qiskit
qiskit copied to clipboard
Move circuit drawer files to `qiskit.visualization.circuit`
Summary
Moves circuit drawer files to circuit
sub-directory
Details and comments
This PR adds a new circuit
sub-directory to the qiskit.visualization
directory and moves all circuit drawer modules along with styles
and the circuit-related utility functions to the circuit
dir.
These changes were also made,
- Two functions in the
utils.py
file that were only used bystate_visualization
were moved to that module. - All other
utils
functions, except for_trim
andmatplotlib_close_if_inline
which were used by multiple modules, were moved tocircuit_utils
in thecircuit
sub-directory. - The
pulse_visualization.py
file was removed since it had been deprecated for a long time. @nkanazawa1989 confirmed this was ok. - Two stub files -
circuit_visualization
andqcstyle.py
- were left inqiskit.visualization
which just import fromcircuit.circuit_visualization
andcircuit.qcstyle
, respectively, for backward compatibility. - Imports were cleaned up in several modules, including the
qiskit.visualization.__init__
module, which included more use of '.' and '..' for directories.
In a future PR, the pulse_v2
files can be moved to the pulse
dir. When this is completed and all deprecations delays have passed, the visualization
dir will look like this,
visualization
__init__
array.py
bloch.py
counts_visualization.py
dag_visualization.py
exceptions.py
gate_map.py
pass_manager_visualization.py
state_visualization.py
transition_visualization.py
utils.py
-> circuit
-> pulse
-> timeline
Thank you for opening a new pull request.
Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.
While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.
One or more of the the following people are requested to review this:
- @Qiskit/terra-core
- @enavarro51
Pull Request Test Coverage Report for Build 3004156037
- 469 of 585 (80.17%) changed or added relevant lines in 13 files are covered.
- 57 unchanged lines in 4 files lost coverage.
- Overall coverage decreased (-0.07%) to 84.298%
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
---|---|---|---|
qiskit/visualization/circuit_visualization.py | 0 | 1 | 0.0% |
qiskit/visualization/qcstyle.py | 0 | 1 | 0.0% |
qiskit/visualization/transition_visualization.py | 0 | 2 | 0.0% |
qiskit/visualization/state_visualization.py | 19 | 25 | 76.0% |
qiskit/visualization/circuit/_utils.py | 328 | 350 | 93.71% |
qiskit/visualization/circuit/circuit_visualization.py | 72 | 106 | 67.92% |
qiskit/visualization/circuit/qcstyle.py | 28 | 78 | 35.9% |
<!-- | Total: | 469 | 585 |
Files with Coverage Reduction | New Missed Lines | % |
---|---|---|
qiskit/tools/visualization.py | 1 | 0% |
qiskit/visualization/circuit_visualization.py | 1 | 0% |
qiskit/visualization/qcstyle.py | 1 | 0% |
qiskit/visualization/pulse/matplotlib.py | 54 | 0% |
<!-- | Total: | 57 |
Totals | |
---|---|
Change from base Build 2994503304: | -0.07% |
Covered Lines: | 57808 |
Relevant Lines: | 68576 |
💛 - Coveralls
Could you please get another approval since I recently don't maintain the visualization module except for pulse drawer.
Maybe ./.github/CODEOWNERS
needs an update too?
Indeed, I think this is a great occation for deprecating pulseV1 and pulseV2->pulse.
Luciano's technically correct about the
text.py
,latex.py
andmatplotlib.py
files - we should put stubs in for them.
I'm not sure I understand the reason for this. Currently this PR creates a stub for circuit_visualization
, but only so the user can do from qiskit.visualization import circuit_drawer
. I don't know what the use case would be for loading text.py
, etc. since the classes in those require that something like the private function _get_layered_instructions
from _utils
has been called first to get the nodes
list. This should only be handled by the user by calling through circuit_drawer
assuming no "violation" of private/public.
I can add the stubs in, but it seems practically superfluous.
Just to clarify on the stubs. Do we need to support both
from qiskit.visualization import text
and
from qiskit.visualization.text import <something>
? Thanks.
Yeah, I agree nobody should be using these things. It's mostly that we publish our deprecation policy, and so we really ought to stick with it. It's part of the reason I like to push to make sure internal functions are explicitly marked as private.
The correct way would be for us to ensure from qiskit.visualization.text import ...
works, but if that's too much effort, let's just ensure from qiskit.visualization import text
works, since that provides a backwards-compatible workaround. I think that's a good enough compromise here.
Ok. Allowing from qiskit.visualization import text
only requires another __init__
entry, where allowing
from qiskit.visualization.text import <something>
requires a separate stub text.py
file. So I'd vote for the former, since it gives the user a way to still access text
without knowing about the circuit
directory, but leaves a cleaner visualization
directory and fewer deprecations at the next step.
Thanks, Jake.