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

Molmer-Sorensen model #104

Closed hartytp closed 10 months ago

hartytp commented 10 months ago

A fit model for Molmer-Sorensen interactions was originally added in https://github.com/OxIonics/ionics_fits/commit/f486eb7f19427fa129a48507dcf90ff07e7789f3

This PR finishes that work.

Status: needs tests. The heuristics are currently pretty minimal, will be improved based on the test results.

Implementation: we calculate the displacement / geometric phase and then use analytic expressions for the populations based on those. We could also have got the populations by doing matrix maths on the propagator. That has some advantages (easy to extend to more ions, can handle non-equal rabi frequencies/mode participation factors), but is a bit more heavyweight. If we do go down that road in the future, we should consider whether we want to use qutip or numpy. My weak feeling is that we should go for numpy since qutip would be a bit of a sledgehammer to crack a nut for this. I want to keep this package relatively lightweight, and qutip is a big dependency. It is also surprisingly slow for some simple things.

hartytp commented 10 months ago

Heuristics and tests in place. This is ready for review / merge.

AUTProgram commented 10 months ago

I've had a quick read through this PR and it looks sensible. Haven't checked the mathematical expressions in detail, but they look similar to what I used, so I'll assume that's all fine. According to @hartytp, he's already done some fits with this model and run the tests, so I'll assume there's also no problem there.

But one thing I noticed was that in https://github.com/OxIonics/ionics_fits/blob/dd6a8bc476cf7766f6d868433e0dff0ded137303/ionics_fits/models/molmer_sorensen.py#L64

it says that 3 qubits are supported, but that doesn't seem to be right?

There's also a few very minor improvements (some of them maybe subjective) that could be made, like removing a zero when indexing (a[0:k] can just be written a[:k]), but I don't think it's worth submitting extra commits now just to change these.

AUTProgram commented 10 months ago

Other than the num_qubits issue mentioned above, I think this is fine to merge. We can fix any problems we find during experiments later.

hartytp commented 10 months ago

I've had a quick read through this PR and it looks sensible. Haven't checked the mathematical expressions in detail, but they look similar to what I used, so I'll assume that's all fine.

Yep - I largely copy-pasted from your code

hartytp commented 10 months ago

it says that 3 qubits are supported, but that doesn't seem to be right?

Good catch. That's probably a mistake - should have been walsh idx. Will fix.

hartytp commented 10 months ago

There's also a few very minor improvements (some of them maybe subjective) that could be made, like removing a zero when indexing (a[0:k] can just be written a[:k]), but I don't think it's worth submitting extra commits now just to change these.

Yeah, I don't have a strong sense of style here. I'm happy to change anything you can be bothered to flag, but also happy to leave these small nit picks as is if no one feels strongly

AUTProgram commented 10 months ago

Ok, cool, let's merge soon then

hartytp commented 10 months ago

Okay, auto-merge on!