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

Ising Model Tools #10

Merged
merged 9 commits into from
Feb 13, 2022
Merged

Ising Model Tools #10

merged 9 commits into from
Feb 13, 2022

Conversation

ccoffrin
Copy link
Member

Working towards #8. Thoughts @zmorrell?

@codecov
Copy link

codecov bot commented Feb 10, 2022

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%) ⬇️

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
Copy link
Collaborator

@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
Copy link
Collaborator

@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
Copy link
Member Author

@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
Copy link
Collaborator

@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

@ccoffrin ccoffrin merged commit 97e5cc1 into main Feb 13, 2022
@ccoffrin ccoffrin deleted the ising-tools branch February 13, 2022 15:29
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

Successfully merging this pull request may close these issues.

None yet

2 participants