Skip to content
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

Expose matrix element calculation #27

Open
cjbe opened this issue Jul 23, 2020 · 20 comments
Open

Expose matrix element calculation #27

cjbe opened this issue Jul 23, 2020 · 20 comments

Comments

@cjbe
Copy link
Member

cjbe commented Jul 23, 2020

It would be useful to expose the matrix element calculations in a user friendly and documented way.

For example, if I want to calculate Stark shifts on state |s> from a laser beam, I need to sum over |<e|d|s>|^2/Delta for all |e> that couple to |s> - this would be easy in the current framework if there was a neat way to evaluate <e|d|s>.
This similarly occurs for Raman rates, like Rabi frequency (<e|d|dn><e|d|up>/Delta summing over all connected |e>) or scattering / dephasing rates.

@RHanley1
Copy link
Member

Is the calc_Epole() function in Ion class not what you are after, or am I missing something? It is in the common.py file.

@cjbe
Copy link
Member Author

cjbe commented Jul 23, 2020

that calculates the information I want, but IMHO is not usefully exposed. The docstring says:

Strictly speaking, we actually store scattering amplitudes (square root of the spontaneous scattering rates), rather than matrix elements, they are somewhat more convenient to work with. The two are related by constants and power of the transition frequencies.

I am not sure what this ambiguous prefactor is.

I am looking for a function that takes the state indices that I want to couple, and gives a dipole matrix element in terms of (eg) e*a0 (so matrix elements are ~unity)

@RHanley1
Copy link
Member

What we do is clarified in one of my pull requests for documentation #10, which describes the physics conventions used in the package. Section 1.4.1 is probably of most relevance. This will explain what is meant by slightly confusing docstring.

@hartytp
Copy link
Contributor

hartytp commented Jul 23, 2020

Triaging, I see a couple of separate issues here..

First off is normalization factors: agree on the correct definition of the dipole matrix elements (IIRC there are a few different conventions in use in the literature); check the physics notes are consistent with it and merge that PR; update the docstring to be clearer (likely just cross-referencing the physics part of the manual); update the code to use the correct normalization factor.

@hartytp
Copy link
Contributor

hartytp commented Jul 23, 2020

Second up is the interface....

For example, if I want to calculate Stark shifts on state |s> from a laser beam, I need to sum over |<e|d|s>|^2/Delta for all |e> that couple to |s> - this would be easy in the current framework if there was a neat way to evaluate <e|d|s>

Are we satisfied with just documenting the members of ion better here? The ePole matrix is intended to be public interface although that's not clear from the docs I'll admit.

Are we satisfied with users writing code like ion.ePole[i, j] or ion.ePole[ion.state(ground_level, M=-1/2), ion.state(P32, M=-1/2) or whatever, or do we want to add an explicit accessor function like ion.get_epole(i, j)? If the latter, what would the call signature be? The current API provides public access to matricies as well as methods to index into them with varying levels of granularity (by state, by level, etc). Would we want to expose an interface that covered every permutation of matricies and access level?

Right now (but I'm happy to be convinced otherwise), I think I'd prefer to just improve the documentation (and add more examples and tests) but not add accessor functions to the main ion class.

I do however feel like calculating Stark shifts is a common enough job -- and definitely the kind of thing that I want this package to do -- that we should provide tools for it.

@hartytp
Copy link
Contributor

hartytp commented Jul 23, 2020

What's quite nice is that right now a lot of the sums you want to do are relatively trivial by using the methods already provided by the ion class to generate slices into the ePole and other matrices, which can be summed over, be used for matrix multiplications, etc

@cjbe
Copy link
Member Author

cjbe commented Jul 23, 2020

What's quite nice is that right now a lot of the sums you want to do are relatively trivial by using the methods already provided by the ion class to generate slices into the ePole and other matrices, which can be summed over, be used for matrix multiplications, etc

This is close to true, but not quite. For example, one needs to manually call calc_ePole() AFAICT, then index into an undocumented variable...

IMHO as well as documenting, it would be really useful to have a dipole_matrix_element(initial state index, final state index) method

@hartytp
Copy link
Contributor

hartytp commented Jul 23, 2020

This is close to true, but not quite. For example, one needs to manually call calc_ePole() AFAICT, then index into an undocumented variable...

Yes, that all needs to be documented. We should also revisit the chain of what calls what and see if it's sensible (e.g. if you pass a B-field into the constructor then it calls set_B otherwise you have to set it manually; set_B updates the matrix elements if they exist but doesn't calculate them if they don't; etc).

Originally I didn't want to get the code to calculate the E/M-pole elements by default to keep the initialization time low. However, IIRC last time I bench marked it, the time it takes to calculate those elements is pretty low* and arguably the time saving isn't worth the hassle of having to call them manually.

  • i.e. orders of magnitude faster than the DJS MatLab code IIRC due to various optimizations

@hartytp
Copy link
Contributor

hartytp commented Jul 23, 2020

IMHO as well as documenting, it would be really useful to have a dipole_matrix_element(initial state index, final state index) method

That feels like API bloat to me. As a compromise, I'd be happy to have ion.get_ePole() so one can do ion.get_ePole()[i, j] or ion.get_ePole()[ion.slice(...), ion.state(...)or whatever. Then we could make theePole` matrix private. That would have the side benefit of allowing us to enforce a copy of the returned matrix, which is generally what one wants (I've had too many bugs related to accessing the array directly).

@cjbe
Copy link
Member Author

cjbe commented Jul 23, 2020

I am happy as long as the accessor function is nicely documented, an ideally returns in standard units (e.g. units of e*a0) rather than something that has to be rescaled by the user to be useful

@hartytp
Copy link
Contributor

hartytp commented Jul 23, 2020

Good!

So, I think the list of actions arising out of this issue are then:

  • Make the various matrices (E, V, etc) all private members with properly documented (units etc) accessor functions provided. I think that these can all copy the internal matricies without introducing unacceptable overhead anywhere, but we should look over the code to make sure. The copying should also be documented.
  • Improve the documentation (see the various suggestions in this issue)
  • Benchmark the construction time with/without calculation of all multi-pole elements. Assuming it's not painful, we should make calculation of them always happen whenever the B-field is set/changed for simplicity
  • implement the various documentation fixes suggested in this issue
  • sort out E-pole normalization (merge physics docs first)

Will that make everyone happy? Volunteers to help with any of this (particularly reviewing the docs/moving into a nicer format than latex)?

@cjbe
Copy link
Member Author

cjbe commented Jul 23, 2020

For point 3:
just calculate them now, and avoid premature optimisation. We can always add an option to the constructor to not calculate this later on if we really need to

@hartytp
Copy link
Contributor

hartytp commented Jul 23, 2020

@cjbe thinking about this more, the API bloat isn't as bad as I'd feared. We just need to have accessors that can support: an index; a slice; a Level. I already do that for populations (https://github.com/OxfordIonTrapGroup/ion_phys/blob/8a4fc96e531332391a871617ed102166d9b5323b/ion_phys/common.py#L204 ) and it's fine.

So, really we should just provide accessors like that for every matrix. That also allows the entire matrix to be returned by passing in slices as: get_xxx(:, :)

  • make method names more consistent (e.g. get_slice rather than slice and unify call signatures/docs where posible)

@cjbe
Copy link
Member Author

cjbe commented Jul 23, 2020

For reference, the correct normalisation for my standard conventions is:

D = np.sqrt( ion.Gamma[i,j] * 3*np.pi*ct.epsilon_0*ct.hbar*ct.c**3/(ion.get_frequency(i,j)**3) )

@hartytp
Copy link
Contributor

hartytp commented Jul 24, 2020

Thanks!

@hartytp
Copy link
Contributor

hartytp commented Jul 24, 2020

@RHanley1 does this agree with the convention you used in your notes?

@RHanley1
Copy link
Member

RHanley1 commented Jul 24, 2020

@RHanley1 does this agree with the convention you used in your notes?

Yes, I think my notes is where @cjbe got it from. The only thing to note here is that what I have written in the notes is D = <j|r|i>. So one needs to multiple what I have defined in the notes by e to get the conventional D = <j|er|i> dipole matrix element.

@hartytp
Copy link
Contributor

hartytp commented Jul 24, 2020

Good! (Sorry for not reviewing the notes yet, they're a big positive contribution so I want to get that done, just haven't found the time).

This is quick so I'll likely get to it over the weekend.

@cjbe
Copy link
Member Author

cjbe commented Jul 24, 2020

Indeed - Ryan's notes were consistent with what I expected, and allowed to me work out what the variables actually were

@hartytp
Copy link
Contributor

hartytp commented Jul 24, 2020

Related: does anyone with with experience setting up documentation builds want to give me a hand converting the notes to markdown and setting the build + hosting up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants