Favoring readability over complexity, revisiting `ChannelCreator` #880

mcdo0486 · 2023-03-16T22:18:09Z

mcdo0486
Mar 16, 2023

TLDR:
Pymeasure framework has an intuitive layout that is especially useful for new python or Pymeasure users. Most users will be making Procedures, not changes to drivers or underlying framework. It is important to favor readability over complexity to keep Pymeasure intuitive. CommonBase.ChannelCreator breaks this intuitive layout pattern. Channels should be explicitly defined as they would be called in a Procedure.

Post:
A strength of Pymeasure is the intuitive layout of parameters, procedures, and drivers. From working with researchers, for a lot of experimentalists Pymeasure is their first python project, and the intuitive at a glance approach that Pymeasure has for procedures and drivers really helps for adoption. Most Pymeasure users will never make driver let alone commit to the upstream Pymeasure repository, but they will be making a lot of procedures and the layout of components in the framework goes a long way for these users.

Just to highlight what I think is the intuitive nature for Procedures

from pymeasure.instruments.keithley import Keithley2400
from pymeasure.experiment import Procedure, FloatParameter

class VoltProcedure(Procedure):

    source_voltage = FloatParameter('Source Voltage', units='V', default=0.)

    DATA_COLUMNS = ['Source Voltage (V)', 'Current (A)']
    sourcemeter = None

    def startup(self):
        self.sourcemeter = Keithley2400("GPIB0::20::INSTR")
        self.sourcemeter.reset()
        
    def execute(self):
        self.sourcemeter.source_voltage = self.source_voltage
        data = {
                'Source Voltage (V)':  self.source_voltage,
                'Current (A)': self.sourcemeter.current
        }
        self.emit('results', data)
                
    def shutdown(self):
        self.sourcemeter.shutdown()

The actual layout of the code in a Procedure is intuitive for new users: there are class attributes like FloatParameter, DATA_COLUMNS is important for the results file, then startup() for setting up instruments, execute() when data is emitted, and finally shutdown() to clean things up. And as an additional bonus, FloatParameter will automatically be integrated as an input, along with data plotting, all into a Pymeasure GUI "for free" which is a huge deal.

from pymeasure.instruments import Instrument
from pymeasure.instruments.validators import strict_range

class Keithley2400(Instrument):

    source_voltage = Instrument.control(
        ":SOUR:VOLT?", ":SOUR:VOLT:LEV %g",
        """ A floating point property that controls the source voltage
        in Volts. """,
        validator=strict_range,
        values=[-210, 210]
    )

For instrument drivers the intuitive layout follows for new users with a twist. The explanation that “Instrument.control() is a static method from the CommonBase class that generates a property for getting and setting values to the instrument” is odd for new users of python. But with this approach we can see at a glance the actual VISA commands that are sent, the help docstring describing the property, and validators for the property values. It is easy to make it click, and then inst.source_voltage = 10 or inst.source_voltage follows intuitively.

It is easy to understand how we got here:

Many instruments have channels
Users are implementing their own channel classes on an instrument by instrument basis
There should be a standard Channel class
Channels share a lot of logic with Instrument
A CommonBase can implement this logic for both classes

That all follows, but the current CommonBase.ChannelCreator implementation removes the intuitive layout from instrument drivers.

class SPD1168X(SPDSingleChannelBase):
    """Represent the Siglent SPD1168X Power Supply.
    """

    channels = Instrument.ChannelCreator(SPDChannel, 1)

It is not obvious that this line is generating inst.ch_1.current_limit = 1 and that is how you interact with this instrument. Only by going through the codebase can you find the answer to how this line works.

class LakeShore224(Instrument):
    """ Represents the Lakeshore 224 Temperature monitor and provides a high-level interface
    for interacting with the instrument. Note that the 224 provides 12 temperature input channels
    (A, B, C1-5, D1-5). This driver makes use of the :ref:`LakeShoreChannels`
    .. code-block:: python
        monitor = LakeShore224('GPIB::1')
        print(monitor.input_A.kelvin)           # Print the temperature in kelvin on sensor A
        monitor.input_A.wait_for_temperature()  # Wait for the temperature on sensor A to stabilize.
    """

    i_ch = Instrument.ChannelCreator(LakeShoreTemperatureChannel,
                                     ['0', 'A', 'B',
                                      'C1', 'C2', 'C3', 'C4', 'C5',
                                      'D1', 'D2', 'D3', 'D4', 'D5'],
                                     prefix='input_')

Even if it is included in the docstring, I don’t believe that this line makes it obvious that monitor.input_D5.kelvin is how things work.

Channels should be explicitly defined without a Channel factory to preserve the intuitive approach to instrument drivers (only an example):

class LakeShore224(Instrument):
    """ Represents the Lakeshore 224 Temperature monitor and provides a high-level interface
    for interacting with the instrument. Note that the 224 provides 12 temperature input channels
    (A, B, C1-5, D1-5). This driver makes use of the :ref:`LakeShoreChannels`
    .. code-block:: python
        monitor = LakeShore224('GPIB::1')
        print(monitor.input_A.kelvin)           # Print the temperature in kelvin on sensor A
        monitor.input_A.wait_for_temperature()  # Wait for the temperature on sensor A to stabilize.
    """

    input_0 = LakeShoreTemperatureChannel(channel='0')
    input_A = LakeShoreTemperatureChannel(channel='A')
    input_B = LakeShoreTemperatureChannel(channel='B')
    input_C1 = LakeShoreTemperatureChannel(channel='C1')
    input_C2 = LakeShoreTemperatureChannel(channel='C2')
    input_C3 = LakeShoreTemperatureChannel(channel='C3')
    input_C4 = LakeShoreTemperatureChannel(channel='C4')
    input_C5 = LakeShoreTemperatureChannel(channel='C5')
    input_D1 = LakeShoreTemperatureChannel(channel='D1')
    input_D2 = LakeShoreTemperatureChannel(channel='D2')
    input_D3 = LakeShoreTemperatureChannel(channel='D3')
    input_D4 = LakeShoreTemperatureChannel(channel='D4')
    input_D5 = LakeShoreTemperatureChannel(channel='D5')

Explicitly creating the channels is a better approach for readability even if there are many channels. And besides, most instruments will likely have much fewer channels then this example.

BenediktBurger · 2023-03-17T09:20:36Z

BenediktBurger
Mar 17, 2023
Maintainer

Thank you for raising the issue.

For more background information: In #718 we discussed the ChannelCreator extensively. Unfortunately many comments are in (hidden) conversations. Especially @msmttchr added the idea of the ChannelCreator.

Ideas behind the ChannelCreator (summarized from memory):

like the property creators, we add Channels information to the class, not the instance.
That way you can analyze a class and know which channels it will have
And you can easily change it in a subclass.
All added channels are added to the channels list as well (which your example won't do!)
All channels are created in the same way (as they need to know the instrument, they need to be added to a channels list etc.)

In the end, the ChannelCreator works similar to the property creator control: A user has to learn these two commands (their parameters and their effect) and interpret them, if he wants to understand the source code. The source code will be consistent among all instruments. With your manual approach, each developer will add the Channels in a different way (as a list, individually etc.), see current state of the code base.

Your example won't work, as you assign a single Channel instance (TemperatureChannel(channel='0')) as a class attribute. Therefore all instances of the instrument will use the same Channel instance!

I see your point in understandability of the code, but I think the advantages overweigh the disadvantages. Especially as we talk about understanding the source code. Good instrument documentation should state how the channels are named and accessed such that users do not have to understand how it works.

I think your issue is more about documentation than about the working of pymeasure.

13 replies

bilderbuchi May 22, 2023
Maintainer

Also, keep in mind that the Channel stuff was a big feature, and took a lot of discussion to converge on a working and acceptable (not perfect) design, and we are glad that we got it merged together!

By the way, how do you typically discover instrument functionality when coding? Documentation/API reference? Docstrings? Autocomplete (IDE or ipython)? Reading the source? AFAIK, autocomplete should include/cover the created channels, so these should be discoverable.

mcdo0486 May 22, 2023
Author

It is a big feature and there is a lot of going on there. Also I think a ChannelFactory should exist for creating tons of channels (more than 24?) but that would be the exception. In the default case ChannelCreator would explicitly declare each channel instead of the factory approach. I probably should make a new post as a more executive summary for readability over complexity. I'm going over again the current instruments that have so far used the factory approach.

I explore current instrument drivers with all those methods: generated pymeasure doc site, looking at docstrings, and source code. I think functionality should be as obvious as possible across those methods so an end user can easily use an instrument in a pymeasure procedure.

BenediktBurger May 22, 2023
Maintainer

Also, keep in mind that the Channel stuff was a big feature, and took a lot of discussion to converge on a working and acceptable (not perfect) design, and we are glad that we got it merged together!

That's the reason I'm hesitant on changing it again. In fact, the topic how to define the channels for an instrument was a major discussion point and we went through some pains to find a solution. The starting point of the ChannelCreator was @msmttchr desire to define the channels at class level, such that you might know, how many channels an instrument gets, while looking at the class, not the instance.

I do not see, what you propose to change.

BenediktBurger May 22, 2023
Maintainer

One of our goals was, to have a list of all channels (or a certain subset) in order to loop over all channels (or that subset) such that you can access the channels either by name (for individual access) or via that list.
This was a compromise, as sometimes one or the other is better and we do not want, that some instruments use a list and other instruments individual names.

mcdo0486 May 22, 2023
Author

I appreciate the amount of work that went into the first implementation, but on the other hand now is the best time before a pip release to see if a change makes sense. I can work on a PR to see if there is traction. I am re-reviewing previous discussion, it looks like @CasperSchippers also had a lot of input, but thanks for outlining a few of the goals of the initial approach.

mcdo0486 · 2023-05-23T20:51:48Z

mcdo0486
May 23, 2023
Author

Ok, supposed to be an executive summary so I'll try and keep this short. Commonly, new researchers are handed an experiment setup with opaque control software and left to figure it out. Besides hitting "go", there is no real visibility into what the instruments are doing. Despite often having no background in programming, they are game to try out pymeasure because of the intuitive drivers like meter.source_voltage = 5 and meter.current, along with building blocks of a pymeasure procedure, and the documentation and source code showing properties along with a descriptive docstring. After I check in a month later the researcher has created a bunch of new procedures and finally has control over their experiment, it is amazing.

So I want to preserve the simplistic pattern for the instrument driver interface as much as possible. The current ChannelCreator is a factory approach to generating channel interfaces, made to hit certain goals after lots of discussion and effort, however those channel interfaces are not obvious in the driver file or the documentation.

Across the 14 instruments that use ChannelCreator the average number of channels is 4.

A couple instruments that have more than 16 channels don't use ChannelCreator, like pymeasure/instruments/anritsu/anritsuMS464xB.py and pymeasure/instruments/attocube/anc300.py , maybe because the channel scaffolding for high n channel instruments is complex.

I think if an instrument has fewer than 16 (?), then each channel should be explicitly declared in the instrument driver.

This branch is a proof of concept on how this can work:
https://github.com/mcdo0486/pymeasure/tree/dev/channel_updates

Commit with changes:
master...mcdo0486:pymeasure:dev/channel_updates

Current lakeshore331:
https://github.com/pymeasure/pymeasure/blob/de143eba83d0de736af9567dcdf8baa6c8c25cda/pymeasure/instruments/lakeshore/lakeshore331.py

Updated lakeshore331:
https://github.com/mcdo0486/pymeasure/blob/517c9ed227ef060ba381c85ed6db66d292b640e6/pymeasure/instruments/lakeshore/lakeshore331.py

Current lakeshore331 docs:
https://pymeasure.readthedocs.io/en/latest/api/instruments/lakeshore/lakeshore331.html

Screenshot up new lakeshore331 docs:

You can see the LakeShore331 driver is demonstrating how ChannelCreator is used in both "factory" mode, passing a list of channels, and "singular" mode, where a single channel is passed to create a channel.

"i_ch" is generated with the "factory" mode.

"output_1" and "output_2" are explicitly defined in "singular mode".

Having "output_1" and "output_2" explicitly defined is much more clean in source code and documentation for the user to know it is a channel interface.

The singular channels can have descriptive docstrings that provide more information about the interface. The docstrings for the singular channels get created by using the class.__repr__ so Sphinx picks it up. There may be a better way of doing that.

9 replies

mcdo0486 May 28, 2023
Author

Do I see it correctly, that the only change is, that "Single Channels" are added to a collection as well?

Right now, you can define a singe channel ("input = Channel Creator" will turn to "input == some_channel").

In your proposal, this single channel will end up in a collection (defaults to channel) as well. That is the only (effective) change.

Am I right?

That seems like a small change in behavior.

One thing, we should keep in mind: there should not be too many ways how you could do something, as it makes the executing code)(in common base) more complicated and the using Code (in the instruments) more heterogeneous.

I have to think about it.

Overall I do think it is a small change, pretty much if a list of channel ids are passed, then generate those channel interfaces with the passed prefix, 'ch_' by default, in factory mode and add them to "channels" collection unless otherwise defined.

If a single channel id is passed, generate a single channel interface and use the defined attribute name for the name of the channel, and add it to the "channels" collection unless otherwise defined. Also allows an a docstring for the channel interface so it shows up in sphinx generated docs.

To your point, yea in current pymeasure, I could do this to create a single channel

class LakeShore331(Instrument):
    output_1 = Instrument.ChannelCreator(LakeShoreHeaterChannel, 1, prefix="output_")

This would create a inst.output_1 channel interface, but the collection output_1 that was set 3 lines earlier would be overwritten and the channel wouldn't be added to any collection. That would be inconsistent behavior with other channels, and it would be a clunky way of defining a single channel interface by requiring both a matching prefix and attribute definition. (also I wouldn't have the option for a docstring on this single channel definition)

I totally agree about not having too many ways of doing things, that's one of the core motivations around defining a standard way to create channels. This change will mean there are two ways ChannelCreator can be used, factory mode with an iterable of channel ids, or single mode if a single channel id is passed. With the benefits of single mode, the docstring and clear channel interfaces in source code, I think that is worth it. Documentation will need to be added that clearly explains how to do the two modes though, which I'll have to add.

bilderbuchi May 29, 2023
Maintainer

Is there a way to obtain/generate the API reference (what I think you mean with "docstrings"?) also with the generator/factory?
I missed that channels currently don't seem to end up in the documentation/API reference (or am I misreading)? That is quite the drawback regarding discoverability. Good that autocomplete should still show them, though!

For me, having two ways to construct the channels, explicit/single and via factory/multi, would be OK in principle. Especially if the behaviour of the generated channels is the same/equivalent (counting API docs as behaviour?).

mcdo0486 May 29, 2023
Author

Is there a way to obtain/generate the API reference (what I think you mean with "docstrings"?) also with the generator/factory? I missed that channels currently don't seem to end up in the documentation/API reference (or am I misreading)? That is quite the drawback regarding discoverability. Good that autocomplete should still show them, though!

Yea, what I mean by "docstrings" is also generated documentation with sphinx that shows up on pymeasure docs site. With this PR you could have a docstring to the channel factory by using the docstring argument:

In order to have a standard for how to "add channels" to instruments in one of two ways, if this PR is accepted, should all instruments with fewer than 16 channels declare those channels in "single mode"? For instance, LakeShore331 would have declarative single channel interfaces, not a mix as I've used in examples so far. I think under 16 works as a standard works, but maybe that number can go up or down though. Probably should pick a number though.

bilderbuchi May 29, 2023
Maintainer

could have a docstring to the channel factory by using the docstring argument:

What I meant is: make sure the created channel attributes and their description/docstring show up in the documentation in the same way as the output channels you show.

mcdo0486 May 29, 2023
Author

could have a docstring to the channel factory by using the docstring argument:

What I meant is: make sure the created channel attributes and their description/docstring show up in the documentation in the same way as the output channels you show.

No, I don't think there is an easy way to generate docs for each generated channel interface in "factory" mode.

BenediktBurger · 2023-06-06T08:28:21Z

BenediktBurger
Jun 6, 2023
Maintainer

Let's formulate goals of the behaviour:

For a ChannelCreator with a single Channel:

the variable name of a ChannelCreator becomes the variable name of the Channel. The ChannelCreator accepts an argument "collection" (defaults to channels), in which to store the Channel additionally.
The docstring of the channel becomes the docstring of the ChannelCreator, such that it is shown in Sphinx

For a ChannelCreator with more than one Channel:

the variable name of the ChannelCreator becomes the collection name.
Individual channels are added with the prefix and id combination.
If there is a single channel class, the docstring of that channel becomes that of the ChannelCreator, such that it is shown in Sphinx. Maybe with the addition, that several channels of that type are generated.

What do you think, @mcdo0486 , @bilderbuchi ?

8 replies

BenediktBurger Jun 8, 2023
Maintainer

Having a separate ChannelFactory for the multi-case seems like a good idea, as it makes it clearer what is happening without inspecting the function's arguments in detail. Also, it neatly resolves a design issue I see, i.e. that the return value of ChannelCreator depends on the single/multi distinction by passed argument.
They both could (and probably should) largey share implementations inside (so one will probably thinly wrap the other), but for the public interface it's two different functions for two different purposes.
So yeah, in favour of having ChannelFactory and ChannelCreator.

I also thought about two different classes due to the reasons you mention, so we should do that. What about ChannelCreator and MultiChannelCreator or ChannelCreatorMultiple?

bilderbuchi Jun 8, 2023
Maintainer

ChannelCreator and MultiChannelCreator

sounds good to me

msmttchr Jun 8, 2023
Maintainer

Being channel usually multi-case, I would consider also the option ChannelCreator and SingleChannelCreator.

mcdo0486 Jun 9, 2023
Author

I've pushed a PR for review: #931

Because the term Channel itself is singular I went with ChannelCreator and MultiChannelCreator. Check out the PR to see if it works with your thinking, I updated current instruments that use ChannelCreator.

BenediktBurger Jun 12, 2023
Maintainer

Thanks.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Favoring readability over complexity, revisiting `ChannelCreator` #880

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Replies: 3 comments 30 replies

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

Select a reply

Favoring readability over complexity, revisiting ChannelCreator #880

mcdo0486 Mar 16, 2023

Replies: 3 comments · 30 replies

BenediktBurger Mar 17, 2023 Maintainer

bilderbuchi May 22, 2023 Maintainer

mcdo0486 May 22, 2023 Author

BenediktBurger May 22, 2023 Maintainer

BenediktBurger May 22, 2023 Maintainer

mcdo0486 May 22, 2023 Author

mcdo0486 May 23, 2023 Author

mcdo0486 May 28, 2023 Author

bilderbuchi May 29, 2023 Maintainer

mcdo0486 May 29, 2023 Author

bilderbuchi May 29, 2023 Maintainer

mcdo0486 May 29, 2023 Author

BenediktBurger Jun 6, 2023 Maintainer

BenediktBurger Jun 8, 2023 Maintainer

bilderbuchi Jun 8, 2023 Maintainer

msmttchr Jun 8, 2023 Maintainer

mcdo0486 Jun 9, 2023 Author

BenediktBurger Jun 12, 2023 Maintainer

Favoring readability over complexity, revisiting `ChannelCreator` #880

mcdo0486
Mar 16, 2023

Replies: 3 comments 30 replies

BenediktBurger
Mar 17, 2023
Maintainer

bilderbuchi May 22, 2023
Maintainer

mcdo0486 May 22, 2023
Author

BenediktBurger May 22, 2023
Maintainer

BenediktBurger May 22, 2023
Maintainer

mcdo0486 May 22, 2023
Author

mcdo0486
May 23, 2023
Author

mcdo0486 May 28, 2023
Author

bilderbuchi May 29, 2023
Maintainer

mcdo0486 May 29, 2023
Author

bilderbuchi May 29, 2023
Maintainer

mcdo0486 May 29, 2023
Author

BenediktBurger
Jun 6, 2023
Maintainer

BenediktBurger Jun 8, 2023
Maintainer

bilderbuchi Jun 8, 2023
Maintainer

msmttchr Jun 8, 2023
Maintainer

mcdo0486 Jun 9, 2023
Author

BenediktBurger Jun 12, 2023
Maintainer