search

OxIonics / ionics_fits

Small python fitting library with an emphasis on Atomic Molecular and Optical Physics
Apache License 2.0
0 stars 0 forks source link

Rabi model: updating heuristics calculation #58

Closed saking2 closed 1 year ago

saking2 commented 1 year ago

Currently, the estimate_parameters() method for the Rabi flop fit fixes the phase of its Sinusoid fit based on the value of the start_excited boolean. This assumes P_readout_e > P_readout_g. If we always define e to to be the higher energy state, this can cause problems in the situation where P_readout_e < P_readout_g. Have added a check of the relative values of P_readout_g and P_readout_e before fixing the phase.

hartytp commented 1 year ago

Ace, thanks! Do we also need a fix here https://github.com/OxIonics/ionics_fits/blob/de2643017e857d81d6b92baadf65c7d0cc5fd70e/ionics_fits/models/rabi.py#L228-L232 ?

hartytp commented 1 year ago

to fix the type annotation issue you might need to just add a note to tell pytype not to worry. The issue is that it's spotted that get_initial_value() can return None, which would break your floating point comparisons. In your specific case, that can't happen because you've set a default value first so we know it will not return None, but pytype can't figure that out. Another option is to add something like

if model_parameters["P_readout_e"].get_initial_value() is None or model_parameters["P_readout_g"].get_initial_value() is None:
    raise ValueError()

which will tell it that it can safely ignore the None case, but that's probably a bit clunky!

hartytp commented 1 year ago

Ideally, can you also add a test case here https://github.com/OxIonics/ionics_fits/blob/de2643017e857d81d6b92baadf65c7d0cc5fd70e/test/test_rabi.py#L31-L48

where the default readout levels are swapped around? That should fail without this PR but pass with it.

saking2 commented 1 year ago

Ideally, can you also add a test case here ... where the default readout levels are swapped around?

added in 1e390aa

hartytp commented 1 year ago

@saking2 the test was failing under CI, as was the fuzzing (poe fuzz). These highlighted a few issues, which I've now fixed.

I'll merge this tomorrow am unless you have any feedback. Then we should be ready to try out with some live data.