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

Improve validation in the cutting reconstruction function #581

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

garrison
Copy link
Member

@garrison garrison commented May 8, 2024

  • I took the first few validations and moved them into the following if block, since the conditionals are a bit redundant. This looked better to me once I wrote everything out.
  • The dict can really be any Mapping, so I use that broader class when calling isinstance. At the same time, I've left dict in the type annotations for the sake of users who are reading the docs but don't know what a "Mapping" is.
  • The thing that prompted this was the last chunk, which validates the number of subexperiments. The lack of this error led to an experience where @ibrahim-shehzad was getting "wrong" results. (It turned out, the observables were different than the ones used when generating the subexperiments; having this check will at least make sure the number of observables makes sense.)

Remaining action items

  • Add tests to bring coverage to 100%

@garrison garrison added the cutting QPD-based circuit cutting code label May 8, 2024
@garrison garrison marked this pull request as ready for review May 9, 2024 20:53
@coveralls
Copy link

coveralls commented May 9, 2024

Pull Request Test Coverage Report for Build 9104575058

Details

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.484%

Totals Coverage Status
Change from base Build 9104465513: 0.01%
Covered Lines: 3510
Relevant Lines: 3676

💛 - Coveralls

raise ValueError(
"If observables is a dictionary, results must also be a dictionary."
)
if observables.keys() != results.keys():
Copy link
Collaborator

Choose a reason for hiding this comment

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

This came about because utility-scale workloads may be run in chunks, saved to disk, and recombined, which means there could very easily be errors during that recombination?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is indeed one motivation. Mostly I just wanted to add this as an additional sanity check when passing over this code, since the below lines implicitly make this assumption.

Copy link
Collaborator

@caleb-johnson caleb-johnson 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, thanks! 👍

@garrison garrison merged commit 67f24cb into main Jun 3, 2024
11 checks passed
@garrison garrison deleted the reconstruct-validation branch June 3, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cutting QPD-based circuit cutting code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants