Add more metrics to resource estimator #801
Conversation
|
|
There was a problem hiding this comment.
Thanks for the contribution @adithyabsk. I left some comments.
...ulation/Simulators/ResourcesEstimator/ResourcesEstimatorWithAdditionalPrimitiveOperations.cs
Show resolved
Hide resolved
|
|
||
| namespace Simulator | ||
| { | ||
| public class ResourcesEstimatorWithAdditionalPrimitiveOperations : ResourcesEstimator |
There was a problem hiding this comment.
The logic in this class should be merged directly with the code in the ResourcesEstimator class.
| return config; | ||
| } | ||
|
|
||
| protected virtual IDictionary<string, IEnumerable<Type>>? AdditionalOperations { get; } |
There was a problem hiding this comment.
This was intentionally made a property to be generic for arbitrary additional operations. I think it's okay to hard code the additional operators and not providing this property when merging it with the ResourcesEstimator class.
| using Microsoft.Quantum.Simulation.Core; | ||
| using Microsoft.Quantum.Simulation.QCTraceSimulatorRuntime; | ||
|
|
||
| namespace Simulator |
There was a problem hiding this comment.
The namespace should be Microsoft.Quantum.Simulation.Simulators
| // public override Task<O> Run<T, I, O>(I args) | ||
| // { | ||
| // var result = base.Run<T, I, O>(args).Result; | ||
| // var name = typeof(T).Name; | ||
| // File.WriteAllText($"{name}.txt", ToTSV()); | ||
| // return Task.Run(() => result); | ||
| // } |
There was a problem hiding this comment.
| // public override Task<O> Run<T, I, O>(I args) | |
| // { | |
| // var result = base.Run<T, I, O>(args).Result; | |
| // var name = typeof(T).Name; | |
| // File.WriteAllText($"{name}.txt", ToTSV()); | |
| // return Task.Run(() => result); | |
| // } |
| // CCNOT(a, b, c); | ||
| // T(a); | ||
| // T(b); | ||
|
|
||
| // Original QDK ResEst. -> 9 Ts | ||
| // New QDK ResEst. -> 1 CCZ, 2 Ts |
There was a problem hiding this comment.
This comment should contain some context of what it is trying to express. Also referring to Original and New estimator may be misleading.
| trow["Sum"] = (double)trow["Sum"] - 4 * (double)androw["Sum"] - 7 * (double)cczrow["Sum"]; | ||
| trow["Max"] = (double)trow["Max"] - 4 * (double)androw["Max"] - 7 * (double)cczrow["Max"]; | ||
|
|
||
| // TODO: update CNOT, QubitClifford, and Measure as well |
There was a problem hiding this comment.
One should measure the cost of the other metrics in ApplyAnd, CCZ and Adjoint ApplyAnd to correctly adjust the cost metrics.
There was a problem hiding this comment.
Best approach is to run a simple Q# project, e.g.,
namespace ResEst {
operation Count() : Unit {
use (a, b, c) = (Qubit(), Qubit(), Qubit());
Microsoft.Quantum.Canon.ApplyAnd(a, b, c);
}
}and run this against the current resources estimator to retrieve the numbers to subtract from the other metrics.
| #region Direct access to counts | ||
| public long CNOT => (long)(double)Data!.Rows!.Find("CNOT")![1]; | ||
| public long QubitClifford => (long)(double)Data!.Rows!.Find("QubitClifford")![1]; | ||
| public long T => (long)(double)Data!.Rows!.Find("T")![1]; | ||
| public long Measure => (long)(double)Data!.Rows!.Find("Measure")![1]; | ||
| public long QubitCount => (long)(double)Data!.Rows!.Find("QubitCount")![1]; | ||
| public long Depth => (long)(double)Data!.Rows!.Find("Depth")![1]; | ||
| public long CCZ => (long)(double)Data!.Rows!.Find("CCZ")![1]; | ||
| public long And => (long)(double)Data!.Rows!.Find("And")![1]; | ||
| public long AdjointAnd => (long)(double)Data!.Rows!.Find("AdjointAnd")![1]; | ||
| #endregion |
There was a problem hiding this comment.
I suggest to remove this part and possibly add it as another pull request that targets on making such properties available.
| public override O Execute<T, I, O>(I args) | ||
| { | ||
| var result = base.Execute<T, I, O>(args); | ||
| Console.WriteLine(""); | ||
| Console.WriteLine("---BEGIN TABLE---"); | ||
| Console.WriteLine(ToTSV()); | ||
| Console.WriteLine("---END TABLE---"); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
| public override O Execute<T, I, O>(I args) | |
| { | |
| var result = base.Execute<T, I, O>(args); | |
| Console.WriteLine(""); | |
| Console.WriteLine("---BEGIN TABLE---"); | |
| Console.WriteLine(ToTSV()); | |
| Console.WriteLine("---END TABLE---"); | |
| return result; | |
| } |
adithyabsk
force-pushed
the
t-abalaji/update-resource-estimator
branch
from
5c38f9c
to
4f3147b
Compare
|
So some quick updates from my end, there were some issues getting a unit test set up that used used the Regardless, trying to bump the version the runtime builds ended up with its own set of issues since it seems that the I've set up a [simple toy branch](https://github.com/adithyabsk/qsharp-runtime/tree/unitTestRepro that demonstrates this. |
|
Doesn't it make sense to move forward with this PR or close it? |
Draft pull request adding additional metrics to resource estimator.