lanl-ansi / QuantumAnnealing.jl

Tools for the Simulation and Execution of Quantum Annealing Algorithms
Other
23 stars 5 forks source link

Ising Model Tools #10

Closed ccoffrin closed 2 years ago

ccoffrin commented 2 years ago

Working towards #8. Thoughts @zmorrell?

codecov[bot] commented 2 years ago

Codecov Report

Merging #10 (22fd233) into main (bfad91f) will decrease coverage by 2.84%. The diff coverage is 64.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #10      +/-   ##
==========================================
- Coverage   89.97%   87.13%   -2.85%     
==========================================
  Files           4        5       +1     
  Lines         409      443      +34     
==========================================
+ Hits          368      386      +18     
- Misses         41       57      +16     
Impacted Files Coverage Δ
src/ising.jl 63.63% <63.63%> (ø)
src/dwave.jl 97.20% <100.00%> (-0.15%) :arrow_down:

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 bfad91f...22fd233. Read the comment docs.

zmorrell commented 2 years ago

@ccoffrin Looks good so far. Let me write up a few tests before we merge it in. One thing I notice is that problem with the test for the noisy x and z bqpjson simulations came up in one of the simulations. It just seems the probability didn't end up high enough (0.657 < 0.7). Do you think we should try to modify the test?

zmorrell commented 2 years ago

@ccoffrin it looks like everything is bug free. It is a little confusing to me that the limit argument doesn't actually give the number of printed states in the case of degeneracies, but more of a lower limit; however it seems to be behaving as intended. This should be good to merge as long as that is the intended behavior

ccoffrin commented 2 years ago

@zmorrell you make a good point on the behavior of the limit argument. I was trying to give it a semantics similar to the limit argument for the print_z_state_probabilities function but I agree the output was not as intuitive as I might have expected.

What do you think of changing the semantics to be the number energy levels that are printed with a default value of say 5?

zmorrell commented 2 years ago

@ccoffrin that seems like a good approach. I would propose a default of 3, since that would be one ground state and 2 levels of excited states. I don't know that people would want much more than that, unless they needed the whole state space, which they can choose with limit=0