-
Notifications
You must be signed in to change notification settings - Fork 86
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
add slope threshold parameter #384
base: main
Are you sure you want to change the base?
Conversation
Dear all, Should I do something else about this pull request? Thanks. |
@@ -3,7 +3,7 @@ | |||
from typing import Any, Tuple | |||
from qcodes.instrument import VisaInstrument | |||
from qcodes.parameters import MultiParameter, Parameter, ParameterWithSetpoints | |||
from qcodes.validators import Arrays, Ints | |||
from qcodes.utils.validators import Arrays, Ints |
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.
This changes the import from the current official location to a deprecated legacy location. Could you change this back
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.
Done.
@@ -86,8 +79,8 @@ def get_raw(self) -> np.ndarray: | |||
""" | |||
assert isinstance(self.instrument, FCA3100) | |||
self.instrument.write('CALCulate:AVERage:STATe 0') | |||
self.instrument.write('ARM:COUN {}'.format(self.instrument.samples_number.get_latest())) | |||
data_str=self.instrument.ask("READ:ARRay? {}".format(self.instrument.samples_number.get_latest())) | |||
self._instrument.write('ARM:COUN {}'.format(self._instrument.samples_number.get_latest())) |
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.
Why this change (from instrument
to _instrument
)
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 reason, fixed it.
|
||
def startread(self): | ||
self.ask("Read?") | ||
return |
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.
This looks partially complete? Should this return the value from ask or is it really meant to discard that and return None
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.
Leftover from debugging.
I removed it
@edumur Sorry for the delay. Has been very busy with other stuff. Left some comments inline. Would you also be able to look into why the tests are failing in CI |
Head branch was pushed to by a user without write access
@jenshnielsen No worries ;) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #384 +/- ##
==========================================
- Coverage 10.49% 10.48% -0.01%
==========================================
Files 138 138
Lines 18378 18380 +2
==========================================
Hits 1928 1928
- Misses 16450 16452 +2 ☔ View full report in Codecov by Sentry. |
Dear qcodes people,
I hope to have done the git things correctly this time.
This pull request simply add two parameters to control the slope threshold.
Thanks.
Étienne.