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

Refactor classical postprocessing in shadows module #2152

Merged
merged 22 commits into from
Jan 26, 2024
Merged

Conversation

natestemen
Copy link
Member

@natestemen natestemen commented Jan 9, 2024

Description

PR to refactor mitiq/shadows/classical_postprocessing.py.

Addresses some issues raised in #2129

Docs

Despite there being tests for the shadows module, as we saw in #2113 the tests do not cover everything and a change can pass tests despite making drastic changes to the results in a tutorial.

shadow tutorial before after
Screenshot 2024-01-10 at 4 21 03 PM Screenshot 2024-01-10 at 4 19 44 PM

Note there is a slight difference in the above plot. I'm not totally sure why.

rshadow tutorial before after
Screenshot 2024-01-10 at 5 42 45 PM Screenshot 2024-01-10 at 4 31 58 PM

All the other plots that I checked look either identical, or very close.

@natestemen natestemen added the shadows classical shadows module label Jan 9, 2024
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (e14a799) 98.20% compared to head (dd61f23) 98.26%.

Files Patch % Lines
mitiq/shadows/shadows_utils.py 69.56% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2152      +/-   ##
==========================================
+ Coverage   98.20%   98.26%   +0.06%     
==========================================
  Files          88       88              
  Lines        4167     4143      -24     
==========================================
- Hits         4092     4071      -21     
+ Misses         75       72       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

i misunderstood how `np.array_split` worked. it uses the
number of splits, as opposed to the size of each batch.

(-1)^x + (-1)^y =/= (-1)^(x + y)
@natestemen natestemen marked this pull request as ready for review January 11, 2024 14:43
@natestemen natestemen self-assigned this Jan 11, 2024
@Misty-W
Copy link
Contributor

Misty-W commented Jan 12, 2024

@Min-Li, we noticed some slight differences in the plots after the re-factor. Is this a concern, or do you think the module is working fine?

@FarLab
Copy link
Contributor

FarLab commented Jan 26, 2024

Hi Nate, I had a look at your PR today by going over the commits and it looks good to me! The only concern is the very small difference in the first plots you show, but that might be some randomness that is not fixed.

@natestemen
Copy link
Member Author

only concern is the very small difference in the first plots

Any idea where that change could be coming from? I have not been able to find it.

@FarLab
Copy link
Contributor

FarLab commented Jan 26, 2024

only concern is the very small difference in the first plots

Any idea where that change could be coming from? I have not been able to find it.

looking at the shadows_tutorial notebook, the seed is fixed, and you didn't change that. So maybe that is not the cause.

Copy link
Contributor

@FarLab FarLab left a comment

Choose a reason for hiding this comment

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

Looks good, not much to add besides the insignificant differences in the plot in the tutorial notebook.

@FarLab FarLab merged commit 81b3f78 into master Jan 26, 2024
27 of 28 checks passed
@Min-Li
Copy link
Contributor

Min-Li commented Feb 11, 2024

@Min-Li, we noticed some slight differences in the plots after the re-factor. Is this a concern, or do you think the module is working fine?

Looks good to me, since there are some changes in the code, for me, this slight change of the figure seems to result from adding/removing stages of random sampling (especially in the quantum measurement). But overall, I don't think there are any issues here.

@natestemen natestemen deleted the nts-shadows-cleanup branch April 17, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shadows classical shadows module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants