- Notifications
You must be signed in to change notification settings - Fork17
Improve the performance of get_detcost() function.#108
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c6896e4 toe6bd1d1Compare
LalehB 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.
Thanks so much,@gopinath-vasalamarri
This optimization is super cool 💯
If you get a chance, could you please also share performance results for larger numbers of shots (10K or 1M) and on other codes (surface code, color code, bivariate bicycle codes)?
| doubleTesseractDecoder::get_detcost( | ||
| size_t d,const std::vector<DetectorCostTuple>& detector_cost_tuples)const { | ||
| double min_cost = INF; | ||
| uint32_t min_det_cost = std::numeric_limits<uint32_t>::infinity(); |
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.
Does thestd::numeric_limits<T>::infinity() applies foruint32_t? if it is only for floating-point types then this could become zero and later we might face division by zero issues!
| error_cost = ec.likelihood_cost / dct.detectors_count; | ||
| if (error_cost < min_cost) { | ||
| error_cost = ec.likelihood_cost; | ||
| if (error_cost < min_cost * dct.detectors_count) { |
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 like that you are replacing the div with mul 👏🏻
| error_cost = ec.likelihood_cost; | ||
| if (error_cost < min_cost * dct.detectors_count) { | ||
| min_cost = error_cost; | ||
| min_det_cost = dct.detectors_count; |
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.
maybe we can have a more discriptive name for this variable, something likemin_cost_det_cnt?
Uh oh!
There was an error while loading.Please reload this page.
At the core of the change -- we are essentially swapping double_divs <-> double_muls in the for loop which, are more efficient CPU Cycle wise.
More information on comparison of before and after performance --
https://docs.google.com/spreadsheets/d/17fL_NVWe7XjwGmdBIts75YFuzUmokUyfd_YMEU9ISXY/edit?gid=0#gid=0