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

Add new passmanager.replace(index, ...) method #3004

Merged
merged 21 commits into from
Oct 5, 2019
Merged

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented Aug 19, 2019

Fixed #2179

This PR introduced PassManager.replace() with the same syntax as append (and adds passmanager[index] = pass as sugar). Also extends the drawer to include the index.

image

@mtreinish mtreinish changed the title passmanager.replace(key, ...) Add new passmanager.replace(key, ...) method Sep 24, 2019
@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Sep 24, 2019
@mtreinish
Copy link
Member

It looks like this needs a local rebase because the changelog has been removed.

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.

From a functional level the code lgtm, but I have a couple of inline comments/questions that I think we should address. Mostly on index vs key and the error message we raise.

qiskit/transpiler/passmanager.py Outdated Show resolved Hide resolved
qiskit/transpiler/passmanager.py Outdated Show resolved Hide resolved
@1ucian0 1ucian0 changed the title Add new passmanager.replace(key, ...) method Add new passmanager.replace(index, ...) method Sep 30, 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.

I was ready to approve this, but then I noticed the release notes were written in markdown, but they need to be rst. We should convert this to rst prior to merging. See: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html for an overview of the syntax.

You can build them locally with tox -edocs now (see: https://github.com/Qiskit/qiskit-terra/blob/master/CONTRIBUTING.md#building-release-notes-locally ) or alternatively we build them in CI now. The built output is hosted as an artifact in azure, see: https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=3120&view=artifacts for the output from the most recent run on this PR.

@mtreinish
Copy link
Member

Also, I do not think the images will work because of how the releases get built in the docs. Reno will parse all the yaml files and build a docutils tree from parsing all the individually listed rst elements in the yaml. The image files never are outside the doc build root so I don't think that they are going to be found via sphinx. There is also the complexity added when we do release the release notes are built via Qiskit/qiskit not the terra repo. Right now it's done manually (which is easy to handle this case), but I've been working on automating that process which will make this much more difficult to handle.

@1ucian0
Copy link
Member Author

1ucian0 commented Sep 30, 2019

It would be great to have support for images, as the visualization module is particularly important for us. Reno seems okey with it (linting passing and creating the report correctly). How do you suggest to solve it for now in this case?

@1ucian0
Copy link
Member Author

1ucian0 commented Oct 1, 2019

I replace the png files with some text that illustrates the images. Let me know if you think looks too obscure...

@1ucian0 1ucian0 requested a review from mtreinish October 2, 2019 14:55
@ajavadia ajavadia mentioned this pull request Oct 4, 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 updating the release notes

@1ucian0 1ucian0 merged commit a934b1f into Qiskit:master Oct 5, 2019
@1ucian0 1ucian0 deleted the 2179 branch October 5, 2019 01:02
faisaldebouni pushed a commit to faisaldebouni/qiskit-terra that referenced this pull request Aug 5, 2020
* replace method

* error handle

* lint

* changelog

* lint

* release note

* key->index

* error message

* lint

* release note

* iamges

* replace images for text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It should be easier to modify pass managers.
3 participants