Ats9870 dev branch2 #338

damazter · 2016-09-20T09:47:23Z

Replaces pr #66

Changes proposed in this pull request:
add a general driver for ATS acquisition cards and a specific driver for the ATS9870
@elrama- @AdriaanRol

This branch is ready to merge, example notebook runs perfectly.
Some changes will be needed to this driver in the future to fix some compatibility issues with the
ATS9360 and ATS9440
@nataliejpg @nulinspiratie
I think those changes are best reviewed as separate pull requests to the master branch.

@alexcjohnson @giulioungaretti

…70-dev-branch

…he code and structure is written. More functions would be nice and several todo items are important before actual implementation

fixed some small errors

…tors to validate that the value is in the values of the dictionary. The driver now calls the validate function of the parameter class to enforce this validation. Solved the problem with sample rates changed the default of some parameters implemented trivial dictionaries. Driver still largely untested

Changes: * Typo in line 345 of ATS.py * Avoiding error handling for 518 (see explanation on github post) * Example notebook for ATS, still at a draft stage

Add a TODO item, remove obsolete variable and revert a change regarding post_trigger_size variable to improve code readability

AdriaanRol · 2016-09-20T15:23:36Z

What I have seen of this driver in action I see no reason not to merge it 👍 . Just awaiting @giulioungaretti or @alexcjohnson for 💃

alexcjohnson

@damazter just a few comments on the AcquisitionController implementation. The rest of it I've had my fingers in before, looks good to me but I'd like to give @giulioungaretti a chance to weigh in before we merge.

alexcjohnson · 2016-09-20T16:02:36Z

qcodes/instrument_drivers/AlazarTech/ATS_acquisition_controllers.py

+        return None
+
+    def fit(self, buf):
+        # Discrete Fourier Transform


This isn't exactly a Fourier Transform... it's just one Fourier component. Is it common usage to refer to a single frequency output this way?

@alexcjohnson
For us, this is very common. During an acquisition, we do not have the (computational) speed to calculate the entire transform. We only really need a single point though, as when we demodulate a signal, our target frequency is fixed and well known.

If other users want to demodulate more frequencies, they would have to write their own acquisition_controller

I wasn't commenting on the functionality, which makes perfect sense, just on the naming. I know I'm not the only one who was puzzled by calling this a Fourier Transform, so I wonder if we could give it a more precise name, like SingleFreq_AcquisitionController perhaps? Again, if it's common usage to call this a Fourier Transform I'm happy to be overruled (but then what will we call it when we make one that returns the full transform, perhaps averaged over many shots?)

@AdriaanRol suggested Demodulation_AcquisitionController so I will change it to something like that. As it is only an example acquisitioncontroller, my feeling about the name is actually not that intense.

alexcjohnson · 2016-09-20T16:06:08Z

qcodes/instrument_drivers/AlazarTech/ATS.py

+                                           instrument_class=AlazarTech_ATS)
+
+    def _get_alazar(self):
+        return self.alazar


It's a little backward to use an underscored method to retrieve a non-underscored attribute :) Can we just set self._alazar and take out this method?

@alexcjohnson
@giulioungaretti
I would like some more help in this point, I have renamed self.alazar to self._alazar
but i want to have the self._get_alazar as an interface function to the children of this class.
Having this method will make it very clear to users that a reference to the alazar can be obtained.

However, this method must be private as it makes no sense to obtain a reference to the alazar across different servers, or to the end user. so would it be ok to leave this function in the code?
(repeating myself, I will rename self.alazar to self._alazar)

OK, my main point is addressed now that you have self._alazar - thanks! But if the only reason to have the getter method is so users can find it more easily (and in the method's docstring you even suggest just using the attribute itself), you could also consider an Attributes section in the class docstring.

I added it to the class docstring, maybe the amount of references to the existence of self._alazar is a bit extreme, but it also cannot hurt

alexcjohnson · 2016-09-20T16:09:10Z

qcodes/instrument_drivers/AlazarTech/ATS_acquisition_controllers.py

+        # make a call to the parent class and by extension, create the parameter
+        # structure of this class
+        super().__init__(name, alazar_name, **kwargs)
+        self.add_parameter("acquisition", get_cmd=self.do_acquisition)


Can we move this (along with do_acquisition and set_acquisitionkwargs) into the AcquisitionController base class?

@alexcjohnson
I have thought about this, but I would like to keep the base class as empty as possible.
I think that many 'real' acquisition controllers will want to do something very different. For example, I expect them to calculate their own acquisition_kwargs such that set_acquisitionkwargs to becomes obsolete in these implementations.

I expect similar things for do_acquisition, maybe other users want to split their acquisition into a different (larger) set of parameters

OK, we can leave it for now and revisit this as people add more controllers and we see how this structure varies.

@alexcjohnson
I don't know if all acquisition controllers will be uploaded to the qcodes repo, but thnx for keeping it this way.

alexcjohnson · 2016-09-20T16:12:29Z

qcodes/instrument_drivers/AlazarTech/ATS_acquisition_controllers.py

+class DFT_AcquisitionController(AcquisitionController):
+    def __init__(self, name, alazar_name, demodulation_frequency, **kwargs):
+        self.demodulation_frequency = demodulation_frequency
+        self.acquisitionkwargs = {'acquisition_controller': self}


I would not put 'acquisition_controller' in these kwargs - bad things would happen if you ever changed that! just do alazar.acquire(acquisition_controller=self, **self.acquisitionkwargs)

alexcjohnson · 2016-09-20T16:17:12Z

qcodes/instrument_drivers/AlazarTech/ATS_acquisition_controllers.py

+        self.add_parameter("acquisition", get_cmd=self.do_acquisition)
+
+    def set_acquisitionkwargs(self, **kwargs):
+        self.acquisitionkwargs.update(**kwargs)


if you mean update you should call it update_acquisitionkwargs, as set implies replacing them all... but it's not clear to me that this is better than just letting users edit self.acquisitionkwargs directly - which is implied to be allowed since it doesn't start with an underscore. Anyway, this will feel safer when acquisition_controller=self is separated out.

@alexcjohnson
I agree with you here, I have renamed it to update_acquisitionkwargs.
The reason to use update is that it enables users to change a single parameter of the acquisitionkwargs that have been set before.

nulinspiratie · 2016-09-20T20:46:42Z

The pull request I made with some modifications to this branch actually deals with some of the points @alexcjohnson raises. Would it make sense to merge that branch first?

alexcjohnson · 2016-09-20T21:32:26Z

@nulinspiratie #330 I guess? Looks good to me, to first merge that one into this branch (which may be easiest by cherry-picking your commit onto this branch, since this branch has diverged a bit from your starting point) then I can look again, but maybe chat with @damazter first.

damazter · 2016-09-21T08:42:07Z

@nulinspiratie
If you can make a pullrequest into this branch that would be fine.
But I will also be working to resolve as much issues as i can

giulioungaretti · 2016-09-21T07:39:41Z

qcodes/instrument_drivers/AlazarTech/ATS9870.py

+
+class Multiples(validators.Ints):
+    '''
+    requires an integer


Needs proper docstrings IMHO.
"""
Does fooo bar

Args:
divisor
"""

giulioungaretti · 2016-09-21T11:14:40Z

Aside from my minor comments, and in general the lack of docstrings (or proper format) the rest is ok for me. Also almost impossible to say more than style without having a card here!

good job @damazter @elrama- and all the others!

it should be private

damazter · 2016-09-21T15:48:13Z

@alexcjohnson
@giulioungaretti
I think I have adressed al your issues.
If you have any issues left, please inform me

…ast few commits

alexcjohnson · 2016-09-22T02:55:41Z

qcodes/instrument_drivers/AlazarTech/ATS9870.py

+                            " found '" + str(model) + "' instead.")
+
+
+class Multiples(validators.Ints):


nonblocking, but this seems generically useful enough that we could move it into the core validators module.

i'll make a pr for that

alexcjohnson · 2016-09-22T03:21:35Z

@damazter thanks for the changes. I made a few new nonblocking comments, in particular see what you think about my naming comment but I don't want to hold this up any more so have approved my review. Nice job, this is not an easy driver to make and I think this will be quite a powerful framework for these cards!

giulioungaretti · 2016-09-21T09:08:35Z

qcodes/instrument_drivers/AlazarTech/ATS.py

+            'This method should be implemented in a subclass')
+
+
+class TrivialDictionary:


I'd document why you want a TrivalDictionary in this driver.

…ller

damazter · 2016-09-22T13:17:30Z

waiting on pr #339

…ocally one

damazter · 2016-09-22T14:41:03Z

#339 is now incorporated, so I am ready to be merged
💃 to the core developers to merge

damazter and others added 30 commits February 17, 2016 18:12

first commit of the alazar driver, work in progress

9166634

Merge branch 'master' of https://github.com/qdev-dk/Qcodes into ATS98…

46edef6

…70-dev-branch

started work on acquire function, not completed

522c3af

Merge branch 'master' of https://github.com/qdev-dk/Qcodes into ATS98…

adb3c90

…70-dev-branch

finished acquire method (still untested)

b364da9

Merge branch 'master' of https://github.com/qdev-dk/Qcodes into ATS98…

e47fc46

…70-dev-branch

typo and added idea in comments

665901a

the ATS parent class should work now. It is still untested, but all t…

2637f51

…he code and structure is written. More functions would be nice and several todo items are important before actual implementation

added signal_to_volt and get_sample_speed

00c03ec

added file with acquisitioncontroller example

8694caa

added preliminary list of parameters to add

ae1d458

the __init__ of this driver now works,

53e65ec

fixed some small errors

rename trigger_range to external_trigger_range

888b01e

added all remaining parameters

71640bf

first working & lightly tested version

6b0aac7

acquisition parameters are now automatically fetched from the driver

7854340

change default value of uptodateflag in Alazarparameter

6e16aa5

added empty docstrings for config and acquire

3444592

rename dll variable to remove 9870 reference

48818b1

lint & refactor alazar base and 9870

05b6ee8

refactor ATS _call_dll

480df83

a little more cleanup in ATS

762a4fa

Merge branch 'master' into ATS9870-dev-branch

cafca5f

ATS board finding

6e59e06

Merge branch 'master' into ATS9870-dev-branch

7d53106

Minor changes (mostly typos) for ATS instrument to be properly created

c9ae0ab

Fix on config method for ATS

78b6ef2

Quick correction and first draft of an example

48cf48f

Changes: * Typo in line 345 of ATS.py * Avoiding error handling for 518 (see explanation on github post) * Example notebook for ATS, still at a draft stage

refactor: revert previous change

2544758

Add a TODO item, remove obsolete variable and revert a change regarding post_trigger_size variable to improve code readability

alexcjohnson suggested changes Sep 20, 2016

View reviewed changes

damazter mentioned this pull request Sep 21, 2016

Ats9870 dev branch #66

Closed

damazter2 added 2 commits September 21, 2016 10:51

refactor: remove acquisition_controller from acquisitionkwargs

2c04666

refactor: rename set_acquisitionkwargs into update_acquisitionkwargs

c81aff6

giulioungaretti suggested changes Sep 21, 2016

View reviewed changes

damazter2 added 5 commits September 21, 2016 14:36

docs: add docstring for Mupltiples validator class

3eb915a

docs: add all needed docstrings

6fe2f4e

refactor: self.alazar to self._alazar

35489d5

it should be private

docs: add docstring

34668df

docs: add docstrings

b5226ee

fix: change the example notebook to reflact the changes made in the l…

8e86fc7

…ast few commits

alexcjohnson reviewed Sep 22, 2016

View reviewed changes

alexcjohnson approved these changes Sep 22, 2016

View reviewed changes

giulioungaretti approved these changes Sep 22, 2016

View reviewed changes

damazter mentioned this pull request Sep 22, 2016

Feature/multiple validator #339

Merged

damazter2 added 2 commits September 22, 2016 15:10

refactor: DFT_AcquisitionController to Demodulation_AcquisitionContro…

9bbfaca

…ller

docs: add _alazar to class docstring

65328f0

damazter2 added 2 commits September 22, 2016 16:32

Merge branch 'master' into ATS9870-dev-branch2

7e126b0

refactor: use multiples validator from the qcodes core instead of a l…

819470d

…ocally one

alexcjohnson merged commit f856cca into master Sep 22, 2016

alexcjohnson deleted the ATS9870-dev-branch2 branch September 22, 2016 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ats9870 dev branch2 #338

Ats9870 dev branch2 #338

damazter commented Sep 20, 2016

AdriaanRol commented Sep 20, 2016

alexcjohnson left a comment

alexcjohnson Sep 20, 2016

damazter Sep 21, 2016

alexcjohnson Sep 22, 2016

damazter Sep 22, 2016

alexcjohnson Sep 20, 2016

damazter Sep 21, 2016

alexcjohnson Sep 22, 2016

damazter Sep 22, 2016

alexcjohnson Sep 20, 2016

damazter Sep 21, 2016

alexcjohnson Sep 22, 2016

damazter Sep 22, 2016

alexcjohnson Sep 20, 2016

alexcjohnson Sep 20, 2016

damazter Sep 21, 2016

nulinspiratie commented Sep 20, 2016

alexcjohnson commented Sep 20, 2016

damazter commented Sep 21, 2016

giulioungaretti Sep 21, 2016

giulioungaretti commented Sep 21, 2016

damazter commented Sep 21, 2016

alexcjohnson Sep 22, 2016

damazter Sep 22, 2016

alexcjohnson commented Sep 22, 2016

giulioungaretti Sep 21, 2016

damazter commented Sep 22, 2016

damazter commented Sep 22, 2016

		" found '" + str(model) + "' instead.")


		class Multiples(validators.Ints):

		'This method should be implemented in a subclass')


		class TrivialDictionary:

Ats9870 dev branch2 #338

Ats9870 dev branch2 #338

Conversation

damazter commented Sep 20, 2016

AdriaanRol commented Sep 20, 2016

alexcjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulinspiratie commented Sep 20, 2016

alexcjohnson commented Sep 20, 2016

damazter commented Sep 21, 2016

Choose a reason for hiding this comment

giulioungaretti commented Sep 21, 2016

damazter commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Sep 22, 2016

Choose a reason for hiding this comment

damazter commented Sep 22, 2016

damazter commented Sep 22, 2016