Closed marwahaha closed 2 years ago
Yeah I guess it must be the phases. If it happened from split-species then it must be from the electronic part. So it might be easy to check by trying with a super simple 2-level Hamiltonian with no motional degrees so freedom, so that there is essentially only one (off-diagonal) matrix element to compare between the IonSim construction and the manual construction. I unfortunately won't be able to work on it the rest of this week though :(
I tried playing around some more but I'm not sure how to generate the Hamiltonian you mention Neil.
I found the problem; it was with the test. The way that the laser wavelength was defined needed to be changed from how it had been defined pre-split-species, but I hadn't done that properly. This resulted in the parameter $\Delta$ not actually reflecting the real detuning, and hence a disagreement in phases in the manually- and IonSim-constructed Hamiltonians.
For some reason I was only able to get this fix to work on the master branch, so I pushed that as well. To fix-h-test, I've pushed what I would think would be the same thing, but for some reason the test fails. I like @marwahaha's added comments and changes for readability so it would be nice to still incorporate them, but something functional must have changed as well that I haven't figured out.
Merging #93 (ee59311) into master (7fb6d68) will not change coverage. The diff coverage is
n/a
.:exclamation: Current head ee59311 differs from pull request most recent head 72437b3. Consider uploading reports for the commit 72437b3 to get more accurate results
@@ Coverage Diff @@
## master #93 +/- ##
======================================
Coverage 91.5% 91.5%
======================================
Files 15 15
Lines 1166 1166
======================================
Hits 1067 1067
Misses 99 99
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 7fb6d68...72437b3. Read the comment docs.
@neil-glikin is there a way to make IonSim error with that kind of input? It feels like we should prevent people from making this particular mistake.
I've updated this PR, it's ready to go after a review from either of you.
@marwahaha We could, but I actually made it intentional that transitionwavelength
can accept energy levels rather than sublevels as input, since it still makes physical sense to ask what is the energy difference. I think the mistake came from me hastily translating from the old syntax to the new. I could see the argument for this being a mistake that's too easy to make, though.
ok, that makes sense; sublevel transitions are different than level transitions. I think if even we can make the mistake, users will very likely make it.
I tried to add some comments and fix the test. But nothing I tried seemed to work.
What I noticed:
timescale
but didn't notice it improving the issue.Maybe something more fundamental is broken.