HaeffnerLab / IonSim.jl

a simple tool for simulating trapped ion systems
https://ionsim.org
MIT License
70 stars 17 forks source link

[DO NOT MERGE] Switch to unitful #79

Closed marwahaha closed 2 years ago

marwahaha commented 2 years ago

This PR is not finished

codecov[bot] commented 2 years ago

Codecov Report

Merging #79 (ced337c) into master (ddd827c) will decrease coverage by 25.6%. The diff coverage is 60.0%.

:exclamation: Current head ced337c differs from pull request most recent head e9805d5. Consider uploading reports for the commit e9805d5 to get more accurate results

@@            Coverage Diff            @@
##           master     #79      +/-   ##
=========================================
- Coverage    83.6%   58.0%   -25.6%     
=========================================
  Files          15      13       -2     
  Lines        1199    1150      -49     
=========================================
- Hits         1002     667     -335     
- Misses        197     483     +286     
Impacted Files Coverage Δ
src/constants.jl 25.0% <ø> (-65.3%) :arrow_down:
src/lasers.jl 65.2% <ø> (-32.6%) :arrow_down:
src/species/ca40.jl 66.7% <ø> (ø)
src/species/yb171.jl 0.0% <ø> (-30.0%) :arrow_down:
src/vibrational_modes.jl 73.5% <0.0%> (-26.5%) :arrow_down:
src/traps.jl 51.3% <41.7%> (-43.7%) :arrow_down:
src/ions.jl 58.5% <53.8%> (-24.8%) :arrow_down:
src/hamiltonians.jl 77.6% <100.0%> (+0.1%) :arrow_up:
src/ion_configurations.jl 93.4% <100.0%> (-6.6%) :arrow_down:
src/analytic_functions.jl 0.0% <0.0%> (-100.0%) :arrow_down:
... and 12 more

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 ddd827c...e9805d5. Read the comment docs.

neil-glikin commented 2 years ago

Error may lie with the tests in test_dynamics.jl

neil-glikin commented 2 years ago

Was able to get this working for some simple cases including the MS code example snippet on the front page. Only exception was that the units of time in the hamiltonian (including the function defining the time-dependent Rabi frequency and the array of time values passed into the solver) seemed to need to be simple numbers. Is it expected to be possible to make these unitful as well yet? (Am I missing something or is it just not done yet?)

marwahaha commented 2 years ago

I havent made everything forced to be unitful yet. Also, the dynamics code seems to run fastest without units, so I havent fit that in yet. Let me see what I can do

On Mon, May 9, 2022 at 9:14 PM Neil Glikin @.***> wrote:

Was able to get this working for some simple cases including the MS code example snippet on the front page. Only exception was that the units of time in the hamiltonian (including the function defining the time-dependent Rabi frequency and the array of time values passed into the solver) seemed to need to be simple numbers. Is it expected to be possible to make these unitful as well yet? (Am I missing something or is it just not done yet?)

— Reply to this email directly, view it on GitHub https://github.com/HaeffnerLab/IonSim.jl/pull/79#issuecomment-1121763088, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATMNGKO2TEEN7EQAJUROSDVJGZ6VANCNFSM5VLUO5BQ . You are receiving this because you authored the thread.Message ID: @.***>

neil-glikin commented 2 years ago

Maybe we can make it so that the user passes in units (which then also removes the need to use the timescale argument), and then the solver strips away the units on the backend to maintain the speed

marwahaha commented 2 years ago

Closing as won't do (for now)