-
Notifications
You must be signed in to change notification settings - Fork 64
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
Clarify confusion between kraus operators and collapse operators #616
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
I'd suggest that ideally everything is relabeled as a rate so that nothing depends on the sequence length. If the decay rate of some excited state is 0.1/μs, the probability of decay during 1 μs is (approximately) 0.1, but how do we define the probability of decay during a 10 μs sequence, or longer? If you'd like to turn it around we face similar issues: we know that the probability of decay during a 10 μs sequence is |
Thanks Julius, I was also puzzled by these issues. I will implement the replacement of variables with |
warnings.warn( | ||
f"{prob_name} is deprecated. Use {rate_name} instead.", | ||
DeprecationWarning, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that these deprecation warnings won't show up during normal operation. Perhaps that's the way we want things to work, but I figured it was worth noting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "normal" operation ? In a notebook ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, can be a script too. In general, whenever users are using the code directly I think deprecation warnings don't show by default. I've had this happen before, at least.
raise NotImplementedError( | ||
"Cannot include general " | ||
+ "noise in digital- or all-basis." | ||
basis_check("general") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we are renaming "effective" to "general"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what we were doing... It would make more sense to display it at "effective" instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about "general", "arbitrary" or "custom" would make more sense to me. In any case, we are not doing the renaming here so we could defer the decision to a future PR. I'm just afraid that if you change to "general" here and then we decide to go with something else, this might be forgotten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we were already going for "General" in several places, but I find it does not make much sense... This is why I decided to go with "effective" instead
tutorials/classical_simulation/Simulating with effective noise channels.ipynb
Outdated
Show resolved
Hide resolved
@@ -15,25 +15,23 @@ | |||
"\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's weird I thought I had modified this... In the end I don't think I will speak about probabilities - because I think the formula p(t) = \frac{1 - e^{-\gamma t}}{2}
is true for dephasing noise, but is not for depolarizing noise - I don't think we have a formula for the depolarizing probability function of time @dehond ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a formula for the depolarizing probability function of time @dehond ?
I suppose one could describe depolarizing noise as a decay envelope towards some stationary value, but this is only true in some limiting case (the same applies to dephasing noise). The expression given here is the decay of the contrast envelope for a single-atom Ramsey experiment. When interactions are introduced in a system this becomes more complicated, and in general it's not possible to describe it with such a clean analytical expression.
@@ -15,25 +15,23 @@ | |||
"\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah turns out I did not save my last modifications before pushing, there are lots of "prob" left...
@@ -15,25 +15,23 @@ | |||
"\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #11. config_dephasing = SimConfig(noise="dephasing", dephasing_prob=noise_rate)
And I guess we should use dephasing_rate
on these
Reply via ReviewNB
@@ -15,25 +15,23 @@ | |||
"\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -15,25 +15,23 @@ | |||
"\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #3. noise_rates = np.round(np.linspace(0, 0.2, 5), 3)
So I guess we don't have to be restricted from 0 to 0.2 anymore?
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't have to, but I don't think we would see much if going to higher rates.
@@ -15,25 +15,23 @@ | |||
"\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line #11. config_dephasing = SimConfig(noise="dephasing", dephasing_prob=noise_rate)
Same everywhere on this cell
Reply via ReviewNB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the notebook needs to be executed again
It should be good now :) |
View / edit / reply to this conversation on ReviewNB HGSilveri commented on 2023-12-14T10:57:44Z The list does not seem to be rendering correctly on RTD |
View / edit / reply to this conversation on ReviewNB HGSilveri commented on 2023-12-14T10:57:45Z I can see you cleared the outputs. Isn't this notebook taking a bit too long to run for it to be rerun on every docs build? a-corni commented on 2023-12-15T10:57:01Z It takes 28sec to run the whole notebook, that's eventually a bit too long indeed |
It takes 28sec to run the whole notebook, that's eventually a bit too long indeed View entire conversation on ReviewNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice job!
On first sight it looks pretty good to me. Unfortunately I don't have time to review the code now and use it a bit myself. I can do it after Christmas if you'd like, but I can imagine you don't want to wait with the merge until then. One question that comes up: is it possible to set |
Hey Julius, it's now possible to define |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new plots are very nice, great job!
* Bump to version 0.17dev0 * Refactoring `QutipEmulator` class (#602) * Adding Hamiltonian class * fixing type * Modifying docstring * Simplifying refactoring * Simplifying refactoring * Fix test, simplify attributes in QutipEmulator * Restore _eval_times_instruction, def device in ham * delete get_attr * using NoiseModel for config in Hamiltonian * Fixing doc, tests * Move build_operator, add_config to simulation * Fix typing * Bump to v0.17dev1 * Clarify confusion between kraus operators and collapse operators (#616) * Bump version to v0.17dev0 * Delete L0, refactor error message * Delete identity condition, 2nd order approximation * Fixing tests * Fixing test + doc * Create rate attr, deprecate prob attr * Replace prob by rate, err if def of rate and prob * Fixing type * Use dataclasses.fields, run effective noise notebook * Illustrate rate > 1 in notebook * Modifying cell count * Support standalone serialization of `Register` and `RegisterLayout` to the abstract representation + Hashing for coordinate collections (#627) * Implement CoordsCollection superclass to share hash with Register * Incorporating register and layout schemas * Add serialization and deserialization for layout and register * Add new JSON schemas to the MANIFEST * Add extra UTs * Cover both jsonschema versions in CI * Fix legacy jsonschema validation * Test for legacy jsonschema more efficiently * Addressing review comments * FIX: Coordinate matching in `WeightMap.get_qubit_weight_map()` (#631) * Bump version to 0.17dev2 * Explicitly define public symbols for all modules (#630) * Fix pyright import errors * WIP: Attempt to use __all__ * Exposing the appropriate symbols throughout the codebase * Fix typo * Expose relevant submodules * Expose layout classes in pulser.register * [FIX] Restore compatibility with pytest 8.0.0 (#637) * Unrestrict scipy<1.12 (#635) * Bump version to v0.17dev0 * Remove restriction scipy<1.12 * Clip qutip to 4.7.5 * Replace `Register.rotate()` with `Register.rotated()` (#639) * Modify deprecation of total_bottom_detuning to v0.18 (#638) * Reduce imports on main tutorials (#636) * Bump version to 0.17.0 --------- Co-authored-by: a_corni <antoine.cornillot@pasqal.com> Co-authored-by: Antoine Cornillot <61453516+a-corni@users.noreply.github.com>
Main Changes: ab65b7f Refactoring `QutipEmulator` class (#602) 007a0ae Clarify confusion between kraus operators and collapse operators (#616) 80cf640 Support standalone serialization of `Register` and `RegisterLayout` to the abstract representation + Hashing for coordinate collections (#627) f1637ae Explicitly define public symbols for all modules (#630)
Given that qutip.mesolve uses collapse operators and not kraus operators, I propose to delete the trace of kraus operators in our code and documentation.
kraus_ops
tolocal_collapse_ops
inset_config
.Closes #608