search

nulinspiratie / SilQ

Software for quantum control of donor atoms in silicon
Other
6 stars 1 forks source link

added adaptive threshold up proportion function in analysis #203

Closed RostyslavSavytskyy closed 4 years ago

RostyslavSavytskyy commented 4 years ago

For now it is only function not used anywhere yet. Can implement it later into analyse_flips function and NMR_parameter, etc. But can be already tested as it is

RostyslavSavytskyy commented 4 years ago

Thanks Mark for the questions/suggestions, I will come back for them. At the moment I had to merge with other branches, where this adaptive threshold will be used. So want to raise a note that this branch should be pulled the last, i.e. after state_probability in NMR parameter and EDSR parameter branches.

maij commented 4 years ago

Just putting some of my stats thoughts down here for my future reference: The standard deviation of a binomial distribution under the CLT is sigma/sqrt(n) where n = samples and sigma is the intrinsic standard deviation of the binomial distribution: p(1-p) where p is the centre of the peak, e.g. 0.2 or 0.8. The largest value for sigma is when p = 0.5, so sigma = 0.25. I think a conservative choice for the gaussian_kde bandwidth should be 0.25/sqrt(n), taking care to note that n is NOT the shots_per_frequency but the number of samples gathered for the up-proportions of this round.

We could also think about introducing a confidence level on the threshold found, which would have to be determined somehow. This could allow the use of past thresholds with their confidences as weights to be used to inform the current threshold. This is particularly useful if you're using very few samples.

RostyslavSavytskyy commented 4 years ago

Yes, agree - introducing previous values for threshold and adjusting respectively the current value will be beneficial

RostyslavSavytskyy commented 4 years ago

Implemented what you requested. Should be ready to go.

RostyslavSavytskyy commented 4 years ago

I don't think EDSRParameter is necessary here, so should be good to merge without it, but it is needed overall as well.

nulinspiratie commented 4 years ago

I don't think EDSRParameter is necessary here, so should be good to merge without it, but it is needed overall as well.

Okay in that case can you remove it from the changed files?

RostyslavSavytskyy commented 4 years ago

Ok, I removed EDSRParameter and added small corrections after testing on Tallulah. Should be now good to go.

RostyslavSavytskyy commented 4 years ago

Are you still reviewing anything? Should I make more changes?

nulinspiratie commented 4 years ago

I should have time to look at it today

--

Dr. Serwan Asaad

Research associate

Fundamental Quantum Technologies Laboratory

University of New South Wales Sydney, Australia

s.asaad@unsw.edu.au

fqt.unsw.edu.au http://www.fqt.unsw.edu.au/

On Fri, 26 Jun 2020 at 10:38, RostyslavSavytskyy notifications@github.com wrote:

Are you still reviewing anything? Should I make more changes?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/nulinspiratie/SilQ/pull/203#issuecomment-649885502, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUAWHKHOJZCAL3AVIWUOP3RYPUZHANCNFSM4MKMNQ3Q .

RostyslavSavytskyy commented 4 years ago

Let me know if you have any issues with my additions when merging other branches, can clarify/actively modify a few names, etc.