Update Rabi flop fitting

[AUTProgram]

This PR contains work based on the discussion in https://github.com/OxIonics/ionics_fits/issues/31.

Important changes are the following:

• Fitting function has been fixed
• The base Rabi model takes two independent variables, $t{\mathrm{pulse}}$ and $f$, and six parameters, $P(E|e)$, $P(E|g)$, $\Omega$, $\tau$, $t{\mathrm{dead}}$ and $f{\mathrm{offset}}$. The variable $t{\mathrm{pulse}}$ is the nominal duration of driving pulse (not accounting for dead time) and $f$ represents the frequency variable of the driving field
• The model constructor takes an argument start_excited. This should be True if particle initially occupies the excited state and False if it occupies the ground state.

The variable $t_{\mathrm{pulse}}$ has got an obvious meaning. But the variable that determines the detuning of the driving field from resonance of the qubit is a bit harder to name unambiguously. The quantity that is defined easily is $\delta$, which is the true (angular) detuning from resonance and enters the formula for transition probability in Rabi flop. In practice, $\delta$ is usually not known exactly, but rather depends on the true value of resonance frequency when experiment is run and the frequency of driving field. In the model, I have called the quantity that determines the position of resonance freq_offset and the independent variable that would be scanned in an experiment is freq. The detuning that enters the transition probability is then delta = freq - freq_offset. So freq_offset determines the offset of resonance relative to freq = 0 in a scan. I am not completely happy with this terminology, but I think it's at least better than what we had before. The issue is further complicated because in practice we also sometimes scan a parameter like "B-field", which corresponds to changing the qubit frequency, but nominally the scanned parameter doesn't even have units of frequency. If anyone has got a suggestion to make things clearer, feel free to comment.

I have not updated the tests for this fit model yet. But what I noticed is that decay (finite $\tau$) is not picked up correctly during fit. Not sure whether this even works reliably in the version currently on master branch, though. I think we can also improve the heuristics for parameter estimation a bit more (mainly with regards to tau and the readout levels).

[AUTProgram]

I believe I have made all requested changes in the latest commit. The model function for RabiFlop can be used to plot the transition probability as a function of pulse duration and driving frequency on a 2D grid: The variables passed as part of the tuple x to _func are used in numpy functions, so standard broadcasting rules apply to them.

[hartytp]

Cool. Love the plot 😍

Will review later today. Thanks for the work on this @AUTProgram

[AUTProgram]

I still need to fix the pytests and parameter estimation for tau.

[AUTProgram]

More general comment about docstrings for estimate_parameters, which also addresses points raised above in review: I'm not sure how useful it is if docstring for actual models just repeats the docstring for base class Model. Might be better if there's a short summary of the heuristics actually used (FFT for omega, ...) instead. This means users don't have to read the code to figure out how estimates for different parameters are inferred. Would apply not just to RabiFlop here, but also other models like Sinusoid, for example. What do you think @hartytp ?

[hartytp]

More general comment about docstrings for estimate_parameters, which also addresses points raised above in review: I'm not sure how useful it is if docstring for actual models just repeats the docstring for base class Model

I'm sympathetic to this point of view. In general, should subclasses repeat the parent class docstrings? I'm not sure. I do find it annoying to have to trace through an inheritance tree to find a docstring. But I also find it annoying propagating docstring fixes to all subclasses (so in practice this often ends up with inconsistency). @mbirtwell do you have a general view on this?

[hartytp]

Might be better if there's a short summary of the heuristics actually used (FFT for omega, ...) instead. This means users don't have to read the code to figure out how estimates for different parameters are inferred. Would apply not just to RabiFlop here, but also other models like Sinusoid, for example. What do you think @hartytp ?

I am personally against this. IMHO docstrings should be functional. e.g. the user wants to know what they need to use the function, it's inputs and outputs and limitations. But I do not think it's good practice to document the details of the algorithm. This ends up with docstrings that just repeat code (so why bother) and, worse, they usually end up out of date (so the way the docstring claims the algorithm works isn't how it actually works).

[mbirtwell]

More general comment about docstrings for estimate_parameters, which also addresses points raised above in review: I'm not sure how useful it is if docstring for actual models just repeats the docstring for base class Model

I'm sympathetic to this point of view. In general, should subclasses repeat the parent class docstrings? I'm not sure. I do find it annoying to have to trace through an inheritance tree to find a docstring. But I also find it annoying propagating docstring fixes to all subclasses (so in practice this often ends up with inconsistency). @mbirtwell do you have a general view on this?

I'd like us to run a readthedocs like service which would render doc strings as webpages including adding links where other classes are mentioned. Which would make this much easier. I'm not sure when we'll set this up though. Alternatively better go to definition support in your editor can make finding the referenced docstrings easier as a stop gap.

Basically I agree both of the options that you've proposed are a bit annoying so I'm trying to find a third way.

[hartytp]

Thanks @mbirtwell .

@AUTProgram in this case, can we just stick with what's done elsewhere in this project (i.e. copy the base class docstring). We may change that in the future but I'd prefer to do it all in one go rather than doing different things in different files depending on who wrote them.

[mbirtwell]

For ionics_fits getting docs up on readthedocs could happen sooner, if you go ahead with open sourcing it, it'll be pretty easy to get the docs on to public readthedocs. The stumbling block for most of our projects is standing up a private readthedocs.

[AUTProgram]

@hartytp I think this should be ready for merging

[AUTProgram]

Have addressed comments from previous review in latest commits. I'm still not completely happy with the section on variable w in docstring for RabiFlop, but at least the functionality of all classes should be fine.

[hartytp]

Have addressed comments from previous review in latest commits. I'm still not completely happy with the section on variable w in docstring for RabiFlop, but at least the functionality of all classes should be fine.

what are you unhappy about?

[hartytp]

🎆