Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate allowing extra keys in circuit drawer style dict #3105

Merged
merged 21 commits into from
Sep 25, 2019

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Sep 17, 2019

fixes part of #2797

The MPL drawer was ignoring style: {'plotbarrier':...} anyway, so I'm not sure if I should deprecated just in case.

Also, I added an error when style contains element that are not being used.

@mtreinish
Copy link
Member

Setting plot barrier and reverse bits in the style dict was deprecated as part of the 0.7 release and removed in 0.8 (the actual 0.8 patch was: #1567 ). Any leftover documentation or anything else referring to it in the style dict was just an oversight from that.

@1ucian0
Copy link
Member Author

1ucian0 commented Sep 17, 2019

Oh! good to know! I that case I will clean it up. Fixed in f427337

I'm also putting back the visualization cleaning:

self.addCleanup(os.remove, filename)

ajavadia
ajavadia previously approved these changes Sep 19, 2019
@mtreinish
Copy link
Member

I'm fine with the text drawer changes because it's not user facing, just making the internal kwargs consistent with the other drawers. But I'm a bit concerned about the style dict changes I support the goal of making it more clear that people are passing in invalid values (a config dict isn't really a good pattern, they should be kwargs) but I'm concerned about the end user implications. The style dict has behaved this way for a number of releases (since the mpl drawer was introduced) and it'll potentially be abrupt for users to have it stop working. What do you think about doing this softly at first making it warn that invalid args have been passed in and that in the next release this will be a failure?

@1ucian0
Copy link
Member Author

1ucian0 commented Sep 20, 2019

Yeap. What you are saying makes total sense. Fixed in da802e5

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Thanks for updating. But, now that this is emitting a deprecation warning we should add a release note. Also we maybe should mention that passing in invalid options will be fatal in a future release in the warning.

@1ucian0
Copy link
Member Author

1ucian0 commented Sep 25, 2019

should add a release note.

Added in 5f397c8

@1ucian0 1ucian0 added the Changelog: Deprecation Include in "Deprecated" section of changelog label Sep 25, 2019
@mtreinish mtreinish changed the title "plot barriers" option unified across drawers Deprecate allowing extra keys in circuit drawer style dict Sep 25, 2019
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the release note

@1ucian0 1ucian0 merged commit 49a4295 into Qiskit:master Sep 25, 2019
@1ucian0 1ucian0 deleted the 2797_plot_barriers branch September 25, 2019 19:04
kdk pushed a commit that referenced this pull request Sep 26, 2019
* Remove changelog and document automated creation (#3109)

* Remove changelog and document automated creation

With the introduction of the qiskit-bot release automation for terra we
no longer need to manually maintain or keep a changelog in repo. The
github release pages (which we were manually copy and pasting the
changelog into already) take the place of this file. For all future
releases the creation of this page is completely automatic and handled
by the bot. This works by using the git log and PR tags to figure out
which commits belong in the changelog summary. The commit msg summary
(the first line) is used for the changelog based on the tags. If a
longer explanation is necessary that should be handled in the reno
release notes.

Fixes #3077

* Add README section on changelog

* Make tweaks to readme for clarity

* The option fig.tight_layout is triggering warnings in some situations so it is being removed (#3123)

* no fig.tight_layout

* removing the rest of fig.tight_layout()

* check casteable types in Layout  (#3100)

* test

* cast in insinstance

* style

* layouts are already layouts :)

* Travis CI: The sudo: tag is deprecated on Travis (#3134)

https://blog.travis-ci.com/2018-11-19-required-linux-infrastructure-migration

* Fix a syntax error in QuantumCircuit.mirror. (#3138)

* Fix for #3106 gatefacecolor key on circuit diagrams (#3137)

* Issue #3106 gatefacecolor key has no effect on circuit diagrams

If the style property "gatefacecolor" is set to a different value than the default, this color is applied.

Fixes #3106

* Solved style problem

space before : eliminated

* Update plot_state_hinton() formatting (#3093)

Add axes to the image part. It looks like the image axes already exist for non-empty graphs. The change that I made here is to remove if np.any(dataimag != 0): if the desired behavior is to see the axes on graphs that don't contain any elements.

Replace rho with the latex rho

Use standard format for real and imag parts of rho

Fixes #2321

* Set barrier across multi-qubits correctly (#3042)

* Previously it was assumed that specified barrier has
  a single contiguous span across qubits. Qubits for the
  barrier need not be adjacent in the diagram. This commit
  handles this case by adding barrier across correct qubits.

* compute multiple short spans.

Fixes #2918

* Test pulse clipping fix Osx #1. (#3144)

* Remove superfluous reference to timeslots from assemble_schedules (#3139)

* Extend custom multiqubit gates over classical bits for mpl (#3062)

* Got it to extend over the clasical bits

* Fixed a small bug

* Removed a file accidentally modified

* Fix linter

* Deprecate allowing extra keys in circuit drawer style dict (#3105)

* test

* cast in insinstance

* style

* layouts are already layouts :)

* style['plotbarrier'] is being deprecated

* internal param for text drawer renamed to plot_barriers

* error when style contains unknown options

* better error message

* remove #3100, since it was not my intention to be on it

* style

* remove plotbarrier

* unused import

* replace error with warning

* release note

* rochester_layout (#3125)

* Use _append when args already converted and broadcast. (#2936)

* Remove line in init which had been commented out (#3150)

* Fix circuit drawing justification (#3061)

Fixes #2802

qiskit/visualization/utils.py _get_layered_instructions() was erroneous and did not justify left nor right correctly. I have reworked that code which underlies many visual representations of circuits to the user.

* refactored, needs more work on right justification

* Looks like this works for qiskit-terra issue #2802

* May fix first test failure for this pull request, while causing another?

* Draws conditionals on ClassicalRegister correctly now.

* - Occurs check for gate in gates leads to good test results.
- Added a few more self.maxDiff = None while debugging. Should they be pulled out?

* tabs and trailing spaces ;)

* removed the maxDiff's because they make the Style and lint test in Python 3.5 fail

* added tests
modified changelog

* Test case extended to include original issue reported in #2802

* oops, left a maxDiff=None in from testing. Fixed

* changes to utils.py requested by reviewer maddy-tod

* test cases split as requested by reviewer maddy-tod

* added circuit diagrams in comments to new tests

* removed note from changelog and made it a release note per @mtreinish

* made _LayerSpooler a private class

* Revert "Remove changelog and document automated creation (#3109)"

This reverts commit effad12.

* Make _LayerSpooler a subclass of list

The _LayerSpooler class was only ever used as list. While the class was
an independent type it basically just wrapped an internal list. THen
there were helper functions to access and manipulate that list. These
methods just duplicated what python already provides and the list class
itself already implements. This commit fixes this so the _LayerSpooler
itself is a subclass of list so when it's initialized you can just treat
it as the output list it was generating.

* Clean up release note, fix only, not prelude

* un reverted my revert of CONTRIBUTING.md

* refactored, needs more work on right justification

* Looks like this works for qiskit-terra issue #2802

* May fix first test failure for this pull request, while causing another?

* Draws conditionals on ClassicalRegister correctly now.

* - Occurs check for gate in gates leads to good test results.
- Added a few more self.maxDiff = None while debugging. Should they be pulled out?

* tabs and trailing spaces ;)

* removed the maxDiff's because they make the Style and lint test in Python 3.5 fail

* added tests
modified changelog

* Test case extended to include original issue reported in #2802

* oops, left a maxDiff=None in from testing. Fixed

* changes to utils.py requested by reviewer maddy-tod

* test cases split as requested by reviewer maddy-tod

* added circuit diagrams in comments to new tests

* removed note from changelog and made it a release note per @mtreinish

* made _LayerSpooler a private class

* Make _LayerSpooler a subclass of list

The _LayerSpooler class was only ever used as list. While the class was
an independent type it basically just wrapped an internal list. THen
there were helper functions to access and manipulate that list. These
methods just duplicated what python already provides and the list class
itself already implements. This commit fixes this so the _LayerSpooler
itself is a subclass of list so when it's initialized you can just treat
it as the output list it was generating.

* Clean up release note, fix only, not prelude

* put back missing section

* Add partial transpiler support for targeting ion trap backends. (#3122)

* handle reset/barrier/measure (#3130)

* Do not unroll an Rx,Ry,Rxx circuit to U3,CX because of measure. (#3140)
kdk added a commit that referenced this pull request Oct 11, 2019
* Add partial transpiler support for targeting ion trap backends. (#3122)

* handle reset/barrier/measure (#3130)

* Do not unroll an Rx,Ry,Rxx circuit to U3,CX because of measure. (#3140)

* rebase ion-trap branch (#3153)

* Remove changelog and document automated creation (#3109)

* Remove changelog and document automated creation

With the introduction of the qiskit-bot release automation for terra we
no longer need to manually maintain or keep a changelog in repo. The
github release pages (which we were manually copy and pasting the
changelog into already) take the place of this file. For all future
releases the creation of this page is completely automatic and handled
by the bot. This works by using the git log and PR tags to figure out
which commits belong in the changelog summary. The commit msg summary
(the first line) is used for the changelog based on the tags. If a
longer explanation is necessary that should be handled in the reno
release notes.

Fixes #3077

* Add README section on changelog

* Make tweaks to readme for clarity

* The option fig.tight_layout is triggering warnings in some situations so it is being removed (#3123)

* no fig.tight_layout

* removing the rest of fig.tight_layout()

* check casteable types in Layout  (#3100)

* test

* cast in insinstance

* style

* layouts are already layouts :)

* Travis CI: The sudo: tag is deprecated on Travis (#3134)

https://blog.travis-ci.com/2018-11-19-required-linux-infrastructure-migration

* Fix a syntax error in QuantumCircuit.mirror. (#3138)

* Fix for #3106 gatefacecolor key on circuit diagrams (#3137)

* Issue #3106 gatefacecolor key has no effect on circuit diagrams

If the style property "gatefacecolor" is set to a different value than the default, this color is applied.

Fixes #3106

* Solved style problem

space before : eliminated

* Update plot_state_hinton() formatting (#3093)

Add axes to the image part. It looks like the image axes already exist for non-empty graphs. The change that I made here is to remove if np.any(dataimag != 0): if the desired behavior is to see the axes on graphs that don't contain any elements.

Replace rho with the latex rho

Use standard format for real and imag parts of rho

Fixes #2321

* Set barrier across multi-qubits correctly (#3042)

* Previously it was assumed that specified barrier has
  a single contiguous span across qubits. Qubits for the
  barrier need not be adjacent in the diagram. This commit
  handles this case by adding barrier across correct qubits.

* compute multiple short spans.

Fixes #2918

* Test pulse clipping fix Osx #1. (#3144)

* Remove superfluous reference to timeslots from assemble_schedules (#3139)

* Extend custom multiqubit gates over classical bits for mpl (#3062)

* Got it to extend over the clasical bits

* Fixed a small bug

* Removed a file accidentally modified

* Fix linter

* Deprecate allowing extra keys in circuit drawer style dict (#3105)

* test

* cast in insinstance

* style

* layouts are already layouts :)

* style['plotbarrier'] is being deprecated

* internal param for text drawer renamed to plot_barriers

* error when style contains unknown options

* better error message

* remove #3100, since it was not my intention to be on it

* style

* remove plotbarrier

* unused import

* replace error with warning

* release note

* rochester_layout (#3125)

* Use _append when args already converted and broadcast. (#2936)

* Remove line in init which had been commented out (#3150)

* Fix circuit drawing justification (#3061)

Fixes #2802

qiskit/visualization/utils.py _get_layered_instructions() was erroneous and did not justify left nor right correctly. I have reworked that code which underlies many visual representations of circuits to the user.

* refactored, needs more work on right justification

* Looks like this works for qiskit-terra issue #2802

* May fix first test failure for this pull request, while causing another?

* Draws conditionals on ClassicalRegister correctly now.

* - Occurs check for gate in gates leads to good test results.
- Added a few more self.maxDiff = None while debugging. Should they be pulled out?

* tabs and trailing spaces ;)

* removed the maxDiff's because they make the Style and lint test in Python 3.5 fail

* added tests
modified changelog

* Test case extended to include original issue reported in #2802

* oops, left a maxDiff=None in from testing. Fixed

* changes to utils.py requested by reviewer maddy-tod

* test cases split as requested by reviewer maddy-tod

* added circuit diagrams in comments to new tests

* removed note from changelog and made it a release note per @mtreinish

* made _LayerSpooler a private class

* Revert "Remove changelog and document automated creation (#3109)"

This reverts commit effad12.

* Make _LayerSpooler a subclass of list

The _LayerSpooler class was only ever used as list. While the class was
an independent type it basically just wrapped an internal list. THen
there were helper functions to access and manipulate that list. These
methods just duplicated what python already provides and the list class
itself already implements. This commit fixes this so the _LayerSpooler
itself is a subclass of list so when it's initialized you can just treat
it as the output list it was generating.

* Clean up release note, fix only, not prelude

* un reverted my revert of CONTRIBUTING.md

* refactored, needs more work on right justification

* Looks like this works for qiskit-terra issue #2802

* May fix first test failure for this pull request, while causing another?

* Draws conditionals on ClassicalRegister correctly now.

* - Occurs check for gate in gates leads to good test results.
- Added a few more self.maxDiff = None while debugging. Should they be pulled out?

* tabs and trailing spaces ;)

* removed the maxDiff's because they make the Style and lint test in Python 3.5 fail

* added tests
modified changelog

* Test case extended to include original issue reported in #2802

* oops, left a maxDiff=None in from testing. Fixed

* changes to utils.py requested by reviewer maddy-tod

* test cases split as requested by reviewer maddy-tod

* added circuit diagrams in comments to new tests

* removed note from changelog and made it a release note per @mtreinish

* made _LayerSpooler a private class

* Make _LayerSpooler a subclass of list

The _LayerSpooler class was only ever used as list. While the class was
an independent type it basically just wrapped an internal list. THen
there were helper functions to access and manipulate that list. These
methods just duplicated what python already provides and the list class
itself already implements. This commit fixes this so the _LayerSpooler
itself is a subclass of list so when it's initialized you can just treat
it as the output list it was generating.

* Clean up release note, fix only, not prelude

* put back missing section

* Add partial transpiler support for targeting ion trap backends. (#3122)

* handle reset/barrier/measure (#3130)

* Do not unroll an Rx,Ry,Rxx circuit to U3,CX because of measure. (#3140)

* Add global Molmer-Sorenson gate (#3149)

* add global ms gate

* clean up

* Update qiskit/extensions/standard/ms.py

Co-Authored-By: Kevin Krsulich <[email protected]>

* Update qiskit/extensions/standard/ms.py

Co-Authored-By: Kevin Krsulich <[email protected]>

* clean up

* Update qiskit/extensions/standard/ms.py

Co-Authored-By: Lauren Capelluto <[email protected]>

* Update MS docstring
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
* test

* cast in insinstance

* style

* layouts are already layouts :)

* style['plotbarrier'] is being deprecated

* internal param for text drawer renamed to plot_barriers

* error when style contains unknown options

* better error message

* remove Qiskit#3100, since it was not my intention to be on it

* style

* remove plotbarrier

* unused import

* replace error with warning

* release note
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
* Add partial transpiler support for targeting ion trap backends. (Qiskit#3122)

* handle reset/barrier/measure (Qiskit#3130)

* Do not unroll an Rx,Ry,Rxx circuit to U3,CX because of measure. (Qiskit#3140)

* rebase ion-trap branch (Qiskit#3153)

* Remove changelog and document automated creation (Qiskit#3109)

* Remove changelog and document automated creation

With the introduction of the qiskit-bot release automation for terra we
no longer need to manually maintain or keep a changelog in repo. The
github release pages (which we were manually copy and pasting the
changelog into already) take the place of this file. For all future
releases the creation of this page is completely automatic and handled
by the bot. This works by using the git log and PR tags to figure out
which commits belong in the changelog summary. The commit msg summary
(the first line) is used for the changelog based on the tags. If a
longer explanation is necessary that should be handled in the reno
release notes.

Fixes Qiskit#3077

* Add README section on changelog

* Make tweaks to readme for clarity

* The option fig.tight_layout is triggering warnings in some situations so it is being removed (Qiskit#3123)

* no fig.tight_layout

* removing the rest of fig.tight_layout()

* check casteable types in Layout  (Qiskit#3100)

* test

* cast in insinstance

* style

* layouts are already layouts :)

* Travis CI: The sudo: tag is deprecated on Travis (Qiskit#3134)

https://blog.travis-ci.com/2018-11-19-required-linux-infrastructure-migration

* Fix a syntax error in QuantumCircuit.mirror. (Qiskit#3138)

* Fix for Qiskit#3106 gatefacecolor key on circuit diagrams (Qiskit#3137)

* Issue Qiskit#3106 gatefacecolor key has no effect on circuit diagrams

If the style property "gatefacecolor" is set to a different value than the default, this color is applied.

Fixes Qiskit#3106

* Solved style problem

space before : eliminated

* Update plot_state_hinton() formatting (Qiskit#3093)

Add axes to the image part. It looks like the image axes already exist for non-empty graphs. The change that I made here is to remove if np.any(dataimag != 0): if the desired behavior is to see the axes on graphs that don't contain any elements.

Replace rho with the latex rho

Use standard format for real and imag parts of rho

Fixes Qiskit#2321

* Set barrier across multi-qubits correctly (Qiskit#3042)

* Previously it was assumed that specified barrier has
  a single contiguous span across qubits. Qubits for the
  barrier need not be adjacent in the diagram. This commit
  handles this case by adding barrier across correct qubits.

* compute multiple short spans.

Fixes Qiskit#2918

* Test pulse clipping fix Osx Qiskit#1. (Qiskit#3144)

* Remove superfluous reference to timeslots from assemble_schedules (Qiskit#3139)

* Extend custom multiqubit gates over classical bits for mpl (Qiskit#3062)

* Got it to extend over the clasical bits

* Fixed a small bug

* Removed a file accidentally modified

* Fix linter

* Deprecate allowing extra keys in circuit drawer style dict (Qiskit#3105)

* test

* cast in insinstance

* style

* layouts are already layouts :)

* style['plotbarrier'] is being deprecated

* internal param for text drawer renamed to plot_barriers

* error when style contains unknown options

* better error message

* remove Qiskit#3100, since it was not my intention to be on it

* style

* remove plotbarrier

* unused import

* replace error with warning

* release note

* rochester_layout (Qiskit#3125)

* Use _append when args already converted and broadcast. (Qiskit#2936)

* Remove line in init which had been commented out (Qiskit#3150)

* Fix circuit drawing justification (Qiskit#3061)

Fixes Qiskit#2802

qiskit/visualization/utils.py _get_layered_instructions() was erroneous and did not justify left nor right correctly. I have reworked that code which underlies many visual representations of circuits to the user.

* refactored, needs more work on right justification

* Looks like this works for qiskit-terra issue Qiskit#2802

* May fix first test failure for this pull request, while causing another?

* Draws conditionals on ClassicalRegister correctly now.

* - Occurs check for gate in gates leads to good test results.
- Added a few more self.maxDiff = None while debugging. Should they be pulled out?

* tabs and trailing spaces ;)

* removed the maxDiff's because they make the Style and lint test in Python 3.5 fail

* added tests
modified changelog

* Test case extended to include original issue reported in Qiskit#2802

* oops, left a maxDiff=None in from testing. Fixed

* changes to utils.py requested by reviewer maddy-tod

* test cases split as requested by reviewer maddy-tod

* added circuit diagrams in comments to new tests

* removed note from changelog and made it a release note per @mtreinish

* made _LayerSpooler a private class

* Revert "Remove changelog and document automated creation (Qiskit#3109)"

This reverts commit effad12.

* Make _LayerSpooler a subclass of list

The _LayerSpooler class was only ever used as list. While the class was
an independent type it basically just wrapped an internal list. THen
there were helper functions to access and manipulate that list. These
methods just duplicated what python already provides and the list class
itself already implements. This commit fixes this so the _LayerSpooler
itself is a subclass of list so when it's initialized you can just treat
it as the output list it was generating.

* Clean up release note, fix only, not prelude

* un reverted my revert of CONTRIBUTING.md

* refactored, needs more work on right justification

* Looks like this works for qiskit-terra issue Qiskit#2802

* May fix first test failure for this pull request, while causing another?

* Draws conditionals on ClassicalRegister correctly now.

* - Occurs check for gate in gates leads to good test results.
- Added a few more self.maxDiff = None while debugging. Should they be pulled out?

* tabs and trailing spaces ;)

* removed the maxDiff's because they make the Style and lint test in Python 3.5 fail

* added tests
modified changelog

* Test case extended to include original issue reported in Qiskit#2802

* oops, left a maxDiff=None in from testing. Fixed

* changes to utils.py requested by reviewer maddy-tod

* test cases split as requested by reviewer maddy-tod

* added circuit diagrams in comments to new tests

* removed note from changelog and made it a release note per @mtreinish

* made _LayerSpooler a private class

* Make _LayerSpooler a subclass of list

The _LayerSpooler class was only ever used as list. While the class was
an independent type it basically just wrapped an internal list. THen
there were helper functions to access and manipulate that list. These
methods just duplicated what python already provides and the list class
itself already implements. This commit fixes this so the _LayerSpooler
itself is a subclass of list so when it's initialized you can just treat
it as the output list it was generating.

* Clean up release note, fix only, not prelude

* put back missing section

* Add partial transpiler support for targeting ion trap backends. (Qiskit#3122)

* handle reset/barrier/measure (Qiskit#3130)

* Do not unroll an Rx,Ry,Rxx circuit to U3,CX because of measure. (Qiskit#3140)

* Add global Molmer-Sorenson gate (Qiskit#3149)

* add global ms gate

* clean up

* Update qiskit/extensions/standard/ms.py

Co-Authored-By: Kevin Krsulich <[email protected]>

* Update qiskit/extensions/standard/ms.py

Co-Authored-By: Kevin Krsulich <[email protected]>

* clean up

* Update qiskit/extensions/standard/ms.py

Co-Authored-By: Lauren Capelluto <[email protected]>

* Update MS docstring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants