Wigner function functionality: Fixed an error#146
Conversation
diego-plan9
left a comment
diego-plan9
left a comment
There was a problem hiding this comment.
No worries, @markjeveritt, glad you caught the indentation issue!
Please note that your PR was issued against a previous version of the master branch on this repository, and was showing changes not belonging to your indentation fix - when issuing a new PR, making sure that you start making your changes from a branch as close to this repository's master branch would be very welcome, as it makes comparison much easier and avoids merging conflicts (and in general makes things much smoother for both the reviewer and the contributor 😄 ).
In practice, it means that your markjeveritt:master branch has been rewritten (and as a result, you will most likely need to clone it again into your local machine for further changes) now contains only two commits compared to our QISKit:master branch:
- 633f6d0 with your indentation fix
- d741ec2 with a set of style changes that I added, in order to conform to the repository style guidelines
I have added some inline notes that should be addressed - most of them seem a result of the renaming of the files and the functions, or similar small issues. Could you please:
- check the issues mentioned on the inline comments, and fix them if needed
- test the Jupyter notebook after addressing the issues, updating your PR on the tutorial repository accordingly if needed
- ... and finally ping us back on this PR?
There was a problem hiding this comment.
Could you double check if the c_index variable is needed, as it seems not used anywhere in the function?
There was a problem hiding this comment.
It is not needed - removing now
There was a problem hiding this comment.
The name parameter is not used in the function - can you check if that is on purpose, and if so, just remove it from the function definition?
| def plot_wigner_data(wigner_data, phis=None, | ||
| thetas=None, method=None, | ||
|
|
||
| def plot_wigner_data(wigner_data, phis=None, thetas=None, method=None, |
There was a problem hiding this comment.
In a similar note to a comment above, can you check if the parameter thetas is needed, as it seems to be unused?
| thetas=None, method=None, | ||
|
|
||
| def plot_wigner_data(wigner_data, phis=None, thetas=None, method=None, | ||
| text_out=None): |
There was a problem hiding this comment.
Can you also document text_out in the docstring?
| method = 'plaquette' | ||
|
|
||
| if method == 'curve': | ||
| plot_wigner_curve(wigner_data, xaxis=phis) |
There was a problem hiding this comment.
In the next lines:
elif method == 'state':
wigner_function(wigner_data, text_out)
wigner_function() seems not to be a valid function name: can you double-check?
There was a problem hiding this comment.
we fixed the function name
@diego-plan9 did we get everything please?
|
Thank you for responding to us so quickly. Sorry about the PR being issued against a previous version of the master branch - I still am getting used to git and did not realise I had done this. Did not mean to cause extra work. Thank you so much for the rewrite of markjeveritt:master branch - we can see it took some effort from you. We have checked the issues mentioned on the inline comments and fixed them. In all cases tis was removing unused variables as you suggested. We have updated and tested the Jupyter notebook. This now works agains updated libraries but note we were only able to confirm against the simulator but not the experiment as the experiments timed out. |
|
@diego-plan9 Ok I am sorry but I am still confused by git - please can you let me know if we need to anything? I think this is the right place to ask/act but please let me know if I have got not wrong. |
|
Thanks for the changes, Mark! They are looking on the right track at a first glance, but I have not done a full review yet - if something else is needed, we make sure to let contributors know by commenting on the PR. In general, it is not unusual that the reviews and the "overall life time" of a pull request spawns over the course of days, depending on the work load and other factors - but we will get to it eventually, no worries! |
|
@diego-plan9 Understood. I am beginning to get git now - thanks for your patience. |
c_index removed name removed from wigner_data thetas removed form plot_wigner_data wigner_function replaced with plot_wigner_function text_out removed from plot_wigner_data as not needed
|
It's looking good, thanks for the contribution, Mark! I'll merge this PR and we can continue discussion if needed on the PR for the tutorials - please note that PR might take longer to get merged, as some changes to that repository are in progress. |
* Pin versions and document versioning policy This commit pins the versions of the tracked elements to ensure that we only ever install a single version of an element when we install the meta-package. Additionally, to make it clear why we do this, and how we handle versioning this also adds a doc on the procedure for handling versioning. Fixes Qiskit#141 * Update contributing guide to point to new docs * Fix missing variable error * Add versioning doc to index
Wigner function functionality: Fixed an error
2 participants
Fix for an indentation error in code.
Description
As above.
Motivation and Context
@diego-plan9 @ChristopheVuillot I am very sorry but an indentation error somehow got into our code. This pull request provides a fix for it.
How Has This Been Tested?
We have downloaded the revised code to a fresh location from GitHub and testing locally.
Screenshots (if appropriate):
Types of changes
Checklist: