-
Notifications
You must be signed in to change notification settings - Fork 19
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rabi frequencies #73
Rabi frequencies #73
Conversation
I think this mostly agrees with experiment, except the quadrupole Rabi frequencies it predicts have been around 2.3 times faster than those found in ABaQuS. I'm not sure whether the discrepancy is due to maths or experimental parameters not being fully known. One thing I maybe suspect is the conversion from Cartesian to Spherical coordinates. The conversion I've done is mostly from https://doi.org/10.1007/s003400050373 with some fudging of signs to make the Rank 1 tensor components agree with what is usually used, but the maths in Section 4.5 of Angular Momentum by Brink and Satchler gives slightly different results. However, it would make the discrepancy bigger. |
56a747f
to
896deea
Compare
896deea
to
8078811
Compare
(Formatting) |
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.
Great to have some work towards handling scattering amplitudes properly etc.
However, a PR along these lines is unlikely to get merged because it doesn't respect and work with some of the basic design decisions behind this package. I'll go into some detail below and in the comments to illustrate what I mean.
In terms of going from here to something mergeable, I'd suggest you try to break this down into a series of smaller PRs which do just one thing (e.g. a small util function for calculating Rayleigh ranges if you feel that's useful) rather than a single large PR which really does a few unconnected things.
Examples of ways this PR doesn't work with the design behind this package:
- we deliberately chose not to have a
State
object because we want to work at arbitrary magnetic fields whereF
is not well-defined (although there is a helper until already provided to aid with this) - we chose to work in saturation intensity units, now power / waist (again, there is a helper util for converting)
- we chose to deal with polarization based on the change in angular momentum for the transition it drives, not polarization angles
- we chose to specify laser frequencies in terms of a detuning from a transition, rather than an absolute wavelength
There is obviously no single convention which is best in every situation, but one has to adopt a representation and work with it, rather than having multiple things which do the same thing in different ways. If you want a different convention, the best approach would be to add a few util functions to help with the conversions - or, if you feel strongly that we should change the fundamental design, to open a discussion issue for one specific point.
Beyond that, the Beam
object really feels like it groups a few things together which don't belong together - just helper functions.
Also, there are no tests! Before merging something like this I'd want to see some tests which exercise the new functionality - e.g. comparing calculations to known good values from literature / trusted theses.
@@ -131,6 +137,11 @@ def __init__( | |||
|
|||
self.levels = levels | |||
self.transitions = transitions | |||
# Store the transitions indexed by the upper and lower levels it connects, | |||
# useful fro reverse searching | |||
self.reverse_transitions = { |
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.
This shouldn't be stored as class attribute - bad practice to store the same information multiple times in different formats.
|
||
return [q_m2, q_m1, q_0, q_p1, q_p2] | ||
|
||
def print_rank1(self, title=None): |
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.
Please remove these print functions. If anything, this is the kind of thing we'd have in an example script, but it doesn't belong in the library code.
@@ -32,6 +32,12 @@ | |||
atom.delta) | |||
""" | |||
|
|||
State = namedtuple("State", ["level", "F", "M"]) | |||
State.__doc__ = """Convenient holder for storing atomic state data. |
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.
Not having a first-class representation of a state was a deliberate decision because this package is intended to work at arbitrary magnetic field, so one cannot generally assume that F
is a good quantum number. Instead, we rely on indexing states within a level by energy order and provide a helper function for getting a best-guess at which state corresponds to a given |F, M_F>
.
Having a first-class object for this would be a much more major change.
from .beam import Beam | ||
import scipy.constants as consts | ||
|
||
__doc__ = """ |
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.
This isn't how we specify module-level docstrings in this package.
import numpy as np | ||
import scipy.constants as consts | ||
|
||
"""Vectors to convert a Cartesian vector to the spherical basis, with q=-1, 0, 1 |
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.
Why docstring formatting when this is basically a comment?
|
||
def __post_init__(self): | ||
self.I_peak = 2 * self.power / (np.pi * self.waist_radius**2) | ||
self.E_field = np.sqrt(2 * self.I_peak / (consts.c * consts.epsilon_0)) |
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.
This should not be a class attribute - trivially calculable from other properties. At most it should be a helper function.
|
||
def scattering_amplitude_to_dipole_matrix_element_prefactor(wavelength: float): | ||
""" | ||
Get the prefactor to convert a scattering amplitude (the scale that |
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.
This description doesn't tell me a lot. I'd like a proper definition which links this to physical observables / gives clear definitions of what we mean (there are a few different conventions around). Linking to papers has a place, but I shouldn't have to read a paper to figure out what this does.
) | ||
|
||
|
||
def scattering_amplitude_to_quadrupole_matrix_element_prefactor(wavelength: float): |
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.
Generally in this package we don't have separate functions for dipole and quadrupole matrix elements - we pass in a transition and figure that out internally.
more complex phase-dependent interactions such as light shifts and Raman | ||
transitions. | ||
|
||
TODO: The quadrupole Rabi frequency disagreed with the experimentally measured |
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.
This kind of TODO does not belong here!
@@ -422,9 +433,7 @@ def calc_Epole(self): | |||
since they are somewhat more convenient to work with. The two are | |||
related by constants and power of the transition frequencies. | |||
|
|||
To do: define what we mean by a matrix element, and how the amplitudes |
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.
Having a helper util for converting to Rabi frequencies is great, but it doesn't remove the need to add some documentation to define what we actually mean by a matrix element and to remind people how this is converted to physically measurable Rabi frequencies (and what we mean by a Rabi frequency since there is more than one convention).
Better to either (a) resolve this kind of discrepancy before opening this PR or (b) explicitly mark the PR as a draft. It's not really clear what the intent behind this PR is - are you looking for a review and merge, or just opening as a placeholder while you do some more thinking? FWIW the sign conventions here are a bit messy. The big thing I was looking for in my todo was to get really clear (and document! e.g. adding a markdown document if needed) on what conventions we're using and avoid fudges. |
NB there is some related discussion in:
Those issues outline the approach I would expect us to take here. |
Thank you for the detailed comments, I appreciate you adding all that info and links. Having read them, I agree with you that the approach I took was not the correct one - and that I probably rushed to raise a PR on something that wasn't ready for that yet. The main intention behind the PR was that I had written this code for some recent simulation work and spent a fair bit of time looking up how to go from the scattering rates in Regarding the TODO on the quadrupole transitions, I agree that this is not in a state that is mergeable - I will find comparisons with literature and include them in tests. I also agree that splitting this up into smaller PRs/issues would make this more manageable. Shall I close this PR, unless you have anything else to add? |
@AVB25 thanks for the contribution. I spent some time over the holiday refreshing the API for atomic physics as well as improving documentation and testing - largely inspired by this PR. I think it's now in a better place to tackle this. If you're still interested in taking this forward, I suggest a good small issue to tackle first would be #86 Do you want to give that a go? |
Another self-contained PR would be to add a helper function to create an arbitrary elliptical polarization based on angles (see the |
Thank you for this! And yes! I'd be more than happy to take both forward. |
Essentially, this is a completion of the TODO in Atom.calc_Epole about the connection between the stored scattering amplitudes and the experimentally measurable Rabi frequencies. Instead of documentation, it adds functions for directly calculating Rabi frequencies and other related quantities like light shift (AC Stark shift) frequencies. There are a couple of other things added, such as a broader
Beam
object to store a laser beam's parameters. This could replace theLaser
object if desired.