Driver for Cryogenic sms120c

[peendebak]

@jenshnielsen @WilliamHPNielsen @layeoh

[WilliamHPNielsen]

Looks like a great driver for a highly non-standard instrument (gotta love the if m[2] == '------->':).

A few issues on style:

• Delete outcommented lines of code
• Use module level logging (log = logging.getLogger(__name__)) and change print to log.error, log.warning, or log.debug where it is appropriate

Optionally, we would be happy if you would use type hints (including return value) for the function.

Is it correct that the parameters can't have units because the instrument swicthes between "Ampere state" and "Tesla state"? If not, the parameters should have a unit.

[layeoh]

Hi William, Thanks for the feedback! I'll make the changes and resubmit. Yes the instrument lets you swap units, so you can run it with Amps or Tesla, though we tend to prefer Tesla. Best regards, LaReine.

…

On Oct 27, 2017 10:30 AM, "William H.P. Nielsen" ***@***.***> wrote: ***@***.**** requested changes on this pull request. Looks like a great driver for a highly non-standard instrument (gotta love the if m[2] == '------->':). A few issues on style: - Delete outcommented lines of code - Use module level logging (log = logging.getLogger(__name__)) and change print to log.error, log.warning, or log.debug where it is appropriate Optionally, we would be happy if you would use type hints (including return value) for the function. Is it correct that the parameters can't have units because the instrument swicthes between "Ampere state" and "Tesla state"? If not, the parameters should have a unit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#819 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AURkE7ZnUYOecPlqYEkK1mvlqod6nD_Dks5swZScgaJpZM4QH7jL> .

[peendebak]

@WilliamHPNielsen Driver now passes tests. @layeoh Can you test the driver?

[layeoh]

Okay great, I'll do a pull on the 'up' branch to sync the changes and test the magnet with the edited code tomorrow in the lab. Thanks for all the help and feedback too by the way!

…

On Oct 29, 2017 9:12 PM, "peendebak" ***@***.***> wrote: @WilliamHPNielsen <https://github.com/williamhpnielsen> Driver now passes tests. @layeoh <https://github.com/layeoh> Can you test the driver? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#819 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AURkEwsiNx4TsXsfzsVKlsUxvpNMml5Jks5sxNxGgaJpZM4QH7jL> .

[WilliamHPNielsen]

It's kind of redundant to maintain such information in the file since we are using git.

[WilliamHPNielsen]

SInce you are now using module level logging (log = logging.getLogger(__name__)), there is no reason to prepend the __name__.

[WilliamHPNielsen]

This should be handled by the connect_message method of the instrument. Unless the IDN response is too non-standard.

[WilliamHPNielsen]

This print call (and many more below in the driver) should be converted to logging calls with the appropriate level.

[layeoh]

Hi ! I have updated the print statements with the log.info as suggested. These statements are important feedback to the user about the state of the magnet, hence the need to print these to the command line. The idn response is quite non standard so it was easier to place it here. The format ive used is similar to that of the ami430 vector magnet driver. Another advantage is perhaps if a different setup uses the same power supply but a different magnet, the next person can modify these properties easily for the magnet and still re-use the driver, as is/minimal modification, to communicate with the power supply.

…

On Nov 16, 2017 9:41 AM, "William H.P. Nielsen" ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In qcodes/instrument_drivers/cryogenic/CryogenicSMS120C.py <#819 (comment)>: > @@ -71,6 +71,7 @@ def __init__(self, name, address, coil_constant=0.113375, current_rating=105.84, idn = self.IDN.get() print(idn) This should be handled by the connect_message method of the instrument. Unless the IDN response is too non-standard. ------------------------------ In qcodes/instrument_drivers/cryogenic/CryogenicSMS120C.py <#819 (comment)>: > units = self._get_unit() if units == 1: - print("Magnet in persistent mode, at a field of %f T" % field) + print("Magnet in persistent mode, at a field of %f T" % This print call (and many more below in the driver) should be converted to logging calls with the appropriate level. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#819 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AURkE2hSUq3-NQeSELps2N-nYoE0nMN3ks5s2_VDgaJpZM4QH7jL> .

[peendebak]

@WilliamHPNielsen Driver should be fine now.

[WilliamHPNielsen]

@layeoh sorry, but I don't really see the requested changes implemented? Perhaps I am missing something, but it looks like you simply left all calls to print as they were?

In short, there should not be any print calls unless in functions that explicitly request it such as print_readable_snapshot or print_error_messages. There should, however, always be logging of such important feedback as "magnet currently holding" etc. It is not up to driver to decide how the user should handle this information, the driver should simply make it available to the user. And printed messages are not available in all circumstances. If somebody is writing automated procedures using an instrument, the output of print is basically lost to them, whereas the logging system messages are easily retrieved. The driver should not make the assumption that users always sit at a terminal and execute driver commands one-by-one.

Finally, I don't understand the purpose of setting the log level to info sometimes. Can you explain the idea to me?

[layeoh]

Hi William, Sorry I think there was a conflict in one of the branches and the update wasn't pushed. Pieter has kindly fixed that, so it should now be the right file. I wanted the log set to info in this case to catch/record these events, since these messages are not programming/communication related, but are more to do with the operation of the magnet and hardware. In some cases like moving in and out of persistent mode, it may take some time, and the user may be wondering what is happening if there is no feedback. Usually magnet PS controller should handle this particular type of basic operation e.g. Switch heater timing for this, but this old model doesn't, so I've added the extra function into the driver for safety/(to ensure correct operation) as it's something that is commonly used in our experiments (and would like to call in an automated routine). Especially when working with equipment like larger superconducting magnets, at full field there is a lot of energy stored/generated, so i prefer the option to get some feedback on the magnet state, at critical points during operation. This log.info 'lower setting' is also optional (can be switched off, reverted back to normal), for other users who do not require it.

…

On Nov 21, 2017 9:31 AM, "William H.P. Nielsen" ***@***.***> wrote: @layeoh <https://github.com/layeoh> sorry, but I don't really see the requested changes implemented? Perhaps I am missing something, but it looks like you simply left all calls to print as they were? In short, there should not be any print calls unless in functions that *explicitly* request it such as print_readable_snapshot or print_error_messages. There should, however, always be logging of such important feedback as "magnet currently holding" etc. It is not up to driver to decide how the user should handle this information, the driver should simply make it available to the user. And printed messages are not available in all circumstances. If somebody is writing automated procedures using an instrument, the output of print is basically lost to them, whereas the logging system messages are easily retrieved. The driver should not make the assumption that users always sit at a terminal and execute driver commands one-by-one. Finally, I don't understand the purpose of setting the log level to info sometimes. Can you explain the idea to me? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#819 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AURkE17dgusHkgSHh32jWqWaol5jsP-cks5s4opTgaJpZM4QH7jL> .

[WilliamHPNielsen]

@layeoh I'm glad we agree about the usefulness of logging over prints! And I can fully get behind always informing about what is happening in the driver, especially if it takes more than a few ms (like moving into persistent mode).

I don't quite understand the way you handle the logging level, though. Basically, I would say that one should never set the logging levels in the driver. If you don't set any levels, all log messages are piped on in the system. Filtering should happen later by setting the level of loghandlers and/or the loggers at runtime.

I plainly don't understand how setting the loglevel to INFO changes anything. Do you actually not get these messages if you don't do that? How do you usually read out logging information during measurement operation?

[layeoh]

Hi William, I'm new to both Python and Qcodes so I'm probably not the best person to answer the part about logging (writing this driver is my introduction to both). I do understand what you mean about the log level handling, and I agree it's certainly not ideal to put this in the driver, but it was the fastest option I could think of at the time of writing the code. At the moment the way logging is handled is just the default python output errors to the command prompt - so by default it only prints warnings and anything more critical than that. (we don't yet have a dedicated logging handler that I can see). So for me to see any log.info output I have to set this level every time. But since we have a lot of routines and instruments, I am reluctant to put it higher up in the code otherwise I might get a flood of messages - I just wanted the log.info level set for this magnet driver only, without disturbing the rest of the workflow (the magnet driver will probably get called at different places in the code by different routines, depending on who is using it). If you have any ideas or a better way to discriminate log level plus its source - or perhaps in general how to handle/approach this particular case, please let me know. - Otherwise it'll will be a lot of setting/unsetting log levels in the experiment scripts, which can get messy quite quickly.

…

On Nov 22, 2017 11:33 AM, "William H.P. Nielsen" ***@***.***> wrote: @layeoh <https://github.com/layeoh> I'm glad we agree about the usefulness of logging over prints! And I can fully get behind always informing about what is happening in the driver, especially if it takes more than a few ms (like moving into persistent mode). I don't quite understand the way you handle the logging level, though. Basically, I would say that one should never set the logging levels in the driver. If you don't set any levels, all log messages are piped on in the system. Filtering should happen later by setting the level of loghandlers and/or the loggers at runtime. I plainly don't understand how setting the loglevel to INFO changes anything. Do you actually not get these messages if you don't do that? How do you usually read out logging information during measurement operation? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#819 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AURkE5FIcEhldnanrCkteQqFSeOnJ7LNks5s4_h3gaJpZM4QH7jL> .

[WilliamHPNielsen]

@layeoh That's a pretty impressive first driver!

Regarding the handling of logging messages, I would recommend the following:

• Delete the setting of logger level in the driver (no surprises here!)
• Do the following (or something very similar) in your scripts/notebooks:

logger = logging.getLogger('<name-of-driver-module>')  # the name is probably 'qcodes/instrument_drivers/cryogenic/CryogenicSMS120C'
handler = logging.StreamHandler(sys.stdout)
logger.setLevel(logging.INFO)  # set the logger's level
handler.setLevel(logging.INFO)  # here you get a second chance to discriminate
formatter = logging.Formatter('%(asctime)s - %(name)s - %(levelname)s - %(message)s')
handler.setFormatter(formatter)
logger.addHandler(handler)

It's probably a bit longer than what you are doing now, but I think that this is more correct. It's also easy to modify to log to a file plus you get to set the format which can be extremely helpful, e.g. by adding information on which function is producing the message (check out https://docs.python.org/3/library/logging.html#logrecord-attributes to see what' spossible).

Does the above help you/make sense?

[peendebak]

@WilliamHPNielsen @layeoh The lines of code above will print each info message twice, but I get the idea.
I still think this is too much code for a typical user to place in any script. The defaults should work for 99% of the users. What about adding

self.handler = logging.StreamHandler(sys.stdout)
if verbose_logging:
   self.handler.setLevel(logging.INFO)  # here you get a second chance to discriminate
formatter = logging.Formatter('%(asctime)s - %(name)s - %(levelname)s - %(message)s')
self.handler.setFormatter(formatter)
logger.addHandler(handler)

to the driver init?

[WilliamHPNielsen]

@peendebak It will only print twice if your root logger (or any parent of the logger) has a handler that prints. But if that is the case, then you should be home safe by simply doing this in your script/notebook:

logger = logging.getLogger('<name-of-driver-module>')  # the name is probably 'qcodes/instrument_drivers/cryogenic/CryogenicSMS120C'
logger.setLevel(logging.INFO)

Does that work for you? @peendebak / @layeoh

[layeoh]

@WilliamHPNielsen i've made the changes with @peendebak today. Thank you for your suggestions, modified the driver module name in the experiment script for it to work, and the write to file option is very handy!

…

On Nov 23, 2017 2:54 PM, "William H.P. Nielsen" ***@***.***> wrote: @peendebak <https://github.com/peendebak> It will only print twice if your root logger has a handler (that prints). But if that is the case, then you should be home safe by simply doing this in your script/notebook: logger = logging.getLogger('<name-of-driver-module>') # the name is probably 'qcodes/instrument_drivers/cryogenic/CryogenicSMS120C' logger.setLevel(logging.INFO) Does that work for you? @peendebak <https://github.com/peendebak> / @layeoh <https://github.com/layeoh> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#819 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AURkE8GGYynRtOUdJPFnB5rBSZmztkd_ks5s5XkDgaJpZM4QH7jL> .

[WilliamHPNielsen]

Looks good to me, I'll just wait for travis and then merge. Thanks for the discussion about logging!

[layeoh]

Great! Thanks very much for your help and feedback @WilliamHPNielsen and @peendebak !