-
Notifications
You must be signed in to change notification settings - Fork 90
Aggregate results in resource estimator across multiple runs #264
Aggregate results in resource estimator across multiple runs #264
Conversation
msoeken
left a comment
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.
I just left some minor comments. I think it would be convenient to have a Clear() or Reset() method on ResourcesEstimator having this new behaviour.
src/Simulation/Simulators/ResourcesEstimator/ResourcesEstimator.cs
Outdated
Show resolved
Hide resolved
How having Clear/Reset is different from creating a new simulator? Does providing this additional functionality (that needs to be documented and tested) make user experience better? I'm not saying we shouldn't do it, but clearing methods are notorious for being confusing to the users and causing subtle bugs. I believe we should do it only if the perf benefits of reusing the simulator to get non-aggregated results provably exist. |
bettinaheim
left a comment
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!
Fixes #189
I'm not sure what the expected behavior is for the "Width" metric. Personally, I'd expect it to report the max of widths across the runs, not adding the per-run widths. However, I'd rather not have special logic for width in the processing loop because it would have to rely on the label and will be fragile, so I've added the max column to the results for all metrics. Is it useful to know the highest count across operations for gates or measurements? The data can be accessed programmatically, as the new test demonstrates, but I didn't update the ToTSV method to include it -- should I? If we go this route the documentation (https://docs.microsoft.com/en-us/quantum/machines/resources-estimator) also will need to be updated.