-
Notifications
You must be signed in to change notification settings - Fork 8
n_coeffs_deriv sorting #83
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
Conversation
Actually I don't remember why the sorting is necessary. Maybe we can drop it? |
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
==========================================
+ Coverage 97.13% 97.14% +0.01%
==========================================
Files 9 9
Lines 2267 2278 +11
Branches 516 520 +4
==========================================
+ Hits 2202 2213 +11
Misses 29 29
Partials 36 36
Continue to review full report at Codecov.
|
The previous sorting was required to comply with the order of control terms. The derivatives have three axes [time, control, noise]. |
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 changes make sense to me and they fix the order of noise hamiltonians as required.
I meant the sorting in general. Why they are stored sorted in the first place. I'll have to think about it, but that can wait. This should work for now. |
Internally operators are sorted by their identifiers, but there were no checks performed in
PulseSequence.get_filter_function_derivative
whether then_coeffs_deriv
parameter is sorted as such.