Skip to content

Add string representation for PassManagerConfig#8085

Merged
mergify[bot] merged 3 commits into
Qiskit:mainfrom
mtreinish:str-pm-config
May 26, 2022
Merged

Add string representation for PassManagerConfig#8085
mergify[bot] merged 3 commits into
Qiskit:mainfrom
mtreinish:str-pm-config

Conversation

@mtreinish
Copy link
Copy Markdown
Member

Summary

This commit adds a new string representation to the PassManagerConfig
class. When str() is run on a PassManagerConfig object it will return a
string listing the values for all of the attributes in the class.

Details and comments

This commit adds a new string representation to the PassManagerConfig
class. When str() is run on a PassManagerConfig object it will return a
string listing the values for all of the attributes in the class.
@mtreinish mtreinish requested a review from a team as a code owner May 18, 2022 20:48
@mtreinish mtreinish added the Changelog: None Do not include in the GitHub Release changelog. label May 18, 2022
@mtreinish mtreinish added this to the 0.21 milestone May 18, 2022
@coveralls
Copy link
Copy Markdown

coveralls commented May 18, 2022

Pull Request Test Coverage Report for Build 2391198920

  • 8 of 9 (88.89%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 84.389%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passmanager_config.py 8 9 88.89%
Totals Coverage Status
Change from base Build 2387377019: 0.02%
Covered Lines: 54551
Relevant Lines: 64642

💛 - Coveralls

jakelishman
jakelishman previously approved these changes May 19, 2022
Copy link
Copy Markdown
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to approve if you're not bothered about the comment.

Comment on lines +129 to +130
return (
"Pass Manager Config:\n"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel very strongly about this at all, but is it worth formatting it as

f"""
PassManagerConfig(
    initial_layout={self.initial_layout},
    basis_gates={self.basis_gates},
...
)
"""

so it can easily be copy-paste evaled back into an interpreter?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's useful to do that in this case because this is not actually going to be a repr string in this case. You won't be able to eval the output because several of the fields are more complex objects that have human readable outputs (InstructionScheduleMap, InstructionDurations, CouplingMap, Target, etc)

I personally was just looking for something that I could use for quick print debugging in #7789 that gave me output I could read to show what was getting passed to the PassManagerConfig

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this - what I was suggesting is more like for repr anyway.

@1ucian0
Copy link
Copy Markdown
Member

1ucian0 commented May 19, 2022

While trying to test it, I realised that I'm not sure if does what is intended:

from qiskit import QuantumRegister
from qiskit.test.mock import FakeMelbourne
from qiskit.transpiler.passmanager_config import PassManagerConfig

qr = QuantumRegister(4, "qr")
initial_layout = [None, qr[0], qr[1], qr[2], None, qr[3]]

backend = FakeMelbourne()
config = PassManagerConfig.from_backend(
    backend, basis_gates=["user_gate"], initial_layout=initial_layout
)

print(config)
Pass Manager Config:
	initial_layout: [None, Qubit(QuantumRegister(4, 'qr'), 0), Qubit(QuantumRegister(4, 'qr'), 1), Qubit(QuantumRegister(4, 'qr'), 2), None, Qubit(QuantumRegister(4, 'qr'), 3)]
	basis_gates: ['user_gate']
	inst_map: None
	coupling_map: [[1, 0], [1, 2], [2, 3], [4, 3], [4, 10], [5, 4], [5, 6], [5, 9], [6, 8], [7, 8], [9, 8], [9, 10], [11, 3], [11, 10], [11, 12], [12, 2], [13, 1], [13, 12]]
	layout_method: None
	routing_method: None
	translation_method: None
	scheduling_method: None
	instruction_durations: id(0,): 5.333333333333333e-08 s
id(1,): 5.333333333333333e-08 s
id(2,): 5.333333333333333e-08 s
id(3,): 5.333333333333333e-08 s
id(4,): 5.333333333333333e-08 s
[...]
measure(11,): 3.555555555555555e-06 s
measure(12,): 3.555555555555555e-06 s
measure(13,): 3.555555555555555e-06 s
measure(14,): 3.555555555555555e-06 s

	backend_properties: <qiskit.providers.models.backendproperties.BackendProperties object at 0x1187d6820>
	approximation_degree: None
	seed_transpiler: None
	timing_constraints: None
	unitary_synthesis_method: default
	unitary_synthesis_plugin_config: None
	target: None

@1ucian0 1ucian0 added the on hold Can not fix yet label May 21, 2022
@1ucian0
Copy link
Copy Markdown
Member

1ucian0 commented May 21, 2022

tagging this as on hold to avoid an accidental merge, given #8085 (comment) and the lack of test. I dont feel very strongly about this, so feel free to remove the tag to dismiss my comment.

@mtreinish
Copy link
Copy Markdown
Member Author

@1ucian0 I'm not sure what your concern is that's the expected output, it's showing the string output for each element in the pass manager config. What are you expecting the output to look like?

I'll add a test, that was an oversight.

@1ucian0
Copy link
Copy Markdown
Member

1ucian0 commented May 25, 2022

For example, instruction_durations are out-of-indent. Also, the output is huge, because the constructor “unfolds” all the backend properties.

@1ucian0 1ucian0 removed the on hold Can not fix yet label May 25, 2022
mtreinish added 2 commits May 26, 2022 09:54
This commit ensure that all entries under the pass manager config have a
single tab indent and also adds a test to verify the output.
@mergify mergify Bot merged commit e76af20 into Qiskit:main May 26, 2022
@mtreinish mtreinish deleted the str-pm-config branch May 26, 2022 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants