-
Notifications
You must be signed in to change notification settings - Fork 26
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
Small alazar fixes #83
Small alazar fixes #83
Conversation
2d06d71
to
3b1eea0
Compare
A couple more comments to the driver.
|
Would it be possible to change the name of 'x' and 'y' to either real/imag or I/Q to keep consistent with the lingo in RF demodulation? |
Sure I have no preference |
I have a couple of request:
|
an update so that we can still use the seq_mode sounds great. I don't think there is much to have an opinion on other then that it should be implemented ;) Otherwise all sounds good. |
although I would make the argument that we should merge the fixes and make another pull request for the outstanding things because of the *0 bug which it's pretty urgent to get fixed in the master branch (I totally underestimated this which caused a lot of time lost for T2 people and I'm trying to learn my lesson) |
In my list the only critical part is: I agree that the rest can follow in a later PR. |
@ThorvaldLarsen can you post or/and explain the warning? I am happy to add this and merge the PR if the warning really does not make any sense in this context and we don't get side effects from setting snapshot_get to false. |
it tries to update alazar_chan.data during the initialization script which it should not be doing. This results in a warning saying that it could not update the snapshot for alazar_chan.data. |
just wondering if there's a reason we haven't merged this yet? |
Yes, the part mentioned in my previous post is not changed yet. |
right, my mistake. If it's not going to get fixed in the next week though I would argue for merging and making a separate pr. I can see it's annoying to get warnings but the *0 fix is so important I don't think it should stay out of the main branch for any reason. |
@Dominik-Vogel is a fix in the next week feasible? If not what's your opinion on merging already? You have the deciding vote ;) |
This is a 5 min thing. So if we don't have time for that then I assume we dont have to merge either. |
5 mins > 1 second |
Well, T2 is open atm so I am sure it can wait a bit longer and then get the changes I requested by my test instead of merging a non-working PR. |
I think it's a really important fix that should be merged as soon as possible and that waiting more than another week isn't an option. |
Add real and imaginary part. Remove strange 0 and add some examples