-
Notifications
You must be signed in to change notification settings - Fork 33
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
Conversation
Pull Request Test Coverage Report for Build 9104575058Details
💛 - Coveralls |
raise ValueError( | ||
"If observables is a dictionary, results must also be a dictionary." | ||
) | ||
if observables.keys() != results.keys(): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! 👍
if
block, since the conditionals are a bit redundant. This looked better to me once I wrote everything out.dict
can really be anyMapping
, so I use that broader class when callingisinstance
. At the same time, I've leftdict
in the type annotations for the sake of users who are reading the docs but don't know what a "Mapping
" is.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