QuTech-Delft / QMI

Quantum Measurement Infrastructure
Other
14 stars 4 forks source link

Add generic QMI power meter #57

Closed rbudhrani closed 7 months ago

rbudhrani commented 10 months ago

Description =========== Having a base class for the power meters in QMI, will generalise power meters so they can be used in the software layers above QMI. A QMI_Powermeter will implement one method called get_power.

For example the existing Thorlabs PM100D power meter will be defined as:

class Thorlabs_PM100D(QMI_Powermeter):
...

QMI_Powermeter will inherit from QMI_Instrument. The following power meters will be redefined to follow this structure:

These power meters will keep their existing definition/API but will also implement the new methods given by QMI_Powermeter

Another solution is to have QMI_Powermeter be a mixin class. This way a concrete QMI instrument can implement multiple mixins. Then the existing Thorlabs PM100D power meter will be defined as:

class Thorlabs_PM100D(QMI_Instrument, QMI_Powermeter):
...

A caveat for this solution is that the metaclass clash between and ABC and QMI_RpcObject must be fixed first.

A third solution would be to Protocols. With this the existing class definitions of the instruments stay the same, but they will need to add the method(s) defined by the protocol. The protocol would be defined as:

class QMI_Powermeter(Protocol):
    def get_power(self) -> float:
        ...

Each QMI_Instrument that would like to adhere to this protocol need only implement the get_power method with the same signature.

The final solution would be to create a concrete version of each power meter based on a base class QMI_Powermeter. Then every power meter would implement a concrete version of QMI_Powermeter. This would decouple the QMI_Instrument from the powermeter of itself.

Modules to be created


Modules to be modified


Tests to be created/updated


Documentation to be updated


Hardware


pieterjbotma commented 10 months ago

Let's stick to just SI units for these abstractions. The conversation can be done in the concrete class. So for powermeter we can stick to Watt.

pieterjbotma commented 10 months ago

The get_idn isn't necessary since it comes with qmi_instrumeny already?

rbudhrani commented 10 months ago

The get_idn isn't necessary since it comes with qmi_instrumeny already?

QMI_Instrument does not have this, so let's leave this out of QMI_Powermeter and add it to QMI_Instrument

rbudhrani commented 10 months ago

Let's stick to just SI units for these abstractions. The conversation can be done in the concrete class. So for powermeter we can stick to Watt.

Agreed

heevasti commented 10 months ago

"QMI_Powermeter will implement one method called get_power."

Don't we need to implement open and close? Or those will always get forwarded to the concrete (inheriting/mixin/protocol) class?

rbudhrani commented 10 months ago

"QMI_Powermeter will implement one method called get_power."

Don't we need to implement open and close? Or those will always get forwarded to the concrete (inheriting/mixin/protocol) class?

open and close will come from QMI_Instrument when using the Protocol/Mixin/Inheriting solution and doesn't need to be implemented again

heevasti commented 9 months ago

OK, some comments:

I like the idea of adding extra category of "power_meter" so that you can the ask context to e.g. show all power meters. We can make the show commands explicit per added new device type, or we could, alternatively, add an optional input to the show_instruments(self, category: Optional[str] = None): self.show_rpc_objects(category) if category else <like now>. That way we do not need to add a new show_xxx for each device type xxx. This would be easiest to implement with the "single inheritance" option. I'm not sure how this would work with the other two, might be tricky. With some "future" thingy but without the need to change anything in the drivers?

I have the following question, which probably could be an issue with the two other options: How about a scenario where we would have a power meter with multiple channels? So, for get_power for that case it would be nice to have directly an input to select the channel number in the call: get_power(channel: Optional[int]: = None), right? But how this would work (or not) in the Mixin and Protocol cases? Or would we forced to make a trick with extra select_channel(channel): self.channel = channel call that would set the channel in self and then in get_power we would then use the self.channel?

So far, the single inheritance looks easiest to implement, but it doesn't enforce any device-type specific behaviour (good or bad?). Probably Mixin would be more future-proof, but I'm not sure how things like the get_category and optional channel input would work there. Somehow I think Protocol would not be flexible in this kind of things and would then require changes in multiple QMI drivers.

heevasti commented 9 months ago

Also, this ticket might potentially have an effect on Add casts to qmi factories.

pieterjbotma commented 9 months ago

@heevasti , regarding the def show_instruments(self, category: Optional[str] = None):, this is already present here.

Therefore, this makes me think that if we go the route of mixins, we need to expand catagory to be a list, since an rpc object can contain multiple categories.

TBH, QMI_Instrument can also just change into a mixin that adds the open, close method to the device.

pieterjbotma commented 9 months ago

Good point about the channel comment. This will also be an "issue" with things like Dac, and DIO. Think about Analog discovery.

But I think we can solve this just by adding an optional channel parameter (channel: Optional[int]) that you can either use or not use in the concrete example.

heevasti commented 9 months ago

@heevasti , regarding the def show_instruments(self, category: Optional[str] = None):, this is already present here.

Therefore, this makes me think that if we go the route of mixins, we need to expand catagory to be a list, since an rpc object can contain multiple categories.

Yes, but the call name list_instruments is nicer and is there already, so we could extend that to be able to take in the category name as well. Well perhaps the list expansion would be a thing to do in the context_singleton version (which is imported as global function in QMI's __init__.py) in https://github.com/QuTech-Delft/QMI/blob/a6cb315f374c1aff6b2a4e784b47cb236b7be8df/qmi/core/context_singleton.py#L360. OR the list_instruments call.

TBH, QMI_Instrument can also just change into a mixin that adds the open, close method to the device.

Why nuts? NOt sure what the exact difference there would be but sounds plausible.

pieterjbotma commented 9 months ago

I would be open for Mixin, seems to make sense since some instruments already have multiple "interfaces".

But then it would be nice to have a look at how to extend the categories to be a list.