search

labrad / pylabrad

python interface for labrad
50 stars 31 forks source link

A bug with multiple type tags with units #393

Open xiaoyuejin opened 2 years ago

xiaoyuejin commented 2 years ago

Python 3.9.8 + pylabrad 0.98.2 here...

When writing a server, I use the format to do type tags like this:

_input = ['v', 'w'] #This is working _input = ['v[mV]', 'w'] #This is working _input = ['v[mV]', 'w[s]'] #This is working

_input = ['v[dBm]', 'v[mV]'] #This is NOT working

It seems that when two (same) float types with (different) units are provided, labrad only recognizes and checks the first unit. As a result, the input with unit of mV in this case will never pass the unit check:

File "C:\LabRAD\WPy64-3980\python-3.9.8.amd64\lib\site-packages\labrad\concurrent.py", line 84, in wrapped result = yield defer.maybeDeferred(func, *args, **kw)

Error: java.lang.IllegalArgumentException: requirement failed: cannot convert units 'mV' to 'dBm'

My current work-around is to bypass the type tags and then test the units separately: _input = ['v'] ... if _input.isCompatible(dBm): ...

But what happens here really looks like a bug to me.

fanmingyu212 commented 2 years ago

This is likely not a bug of pylabrad, but a bug of scalabrad. Here is the error message from labrad.bat:

java.lang.IllegalArgumentException: requirement failed: cannot convert units 'Hz' to 'dBm'
        at scala.Predef$.require(Predef.scala:219)
        at org.labrad.types.Units$.getConversionFactor(Units.scala:134)
        at org.labrad.types.Units$.optConvert(Units.scala:101)
        at org.labrad.types.Units$.convert(Units.scala:112)
        at org.labrad.data.Data$.makeConverter(Data.scala:1235)
        at org.labrad.data.FlatData$$anonfun$convertTo$1.apply(Data.scala:540)
        at org.labrad.data.FlatData$$anonfun$convertTo$1.apply(Data.scala:538)
        at scala.Option.getOrElse(Option.scala:121)
        at org.labrad.data.FlatData.convertTo(Data.scala:537)
        at org.labrad.manager.ServerHandler$$anonfun$2.apply(Handlers.scala:169)
        at org.labrad.manager.ServerHandler$$anonfun$2.apply(Handlers.scala:167)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:245)
        at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:245)
        at scala.collection.immutable.List.foreach(List.scala:381)
        at scala.collection.TraversableLike$class.map(TraversableLike.scala:245)
        at scala.collection.immutable.List.map(List.scala:285)
        at org.labrad.manager.ServerHandler.request(Handlers.scala:167)
        at org.labrad.manager.HubImpl.request(Hub.scala:214)
        at org.labrad.manager.ClientHandler.handleRequest(Handlers.scala:68)
        at org.labrad.manager.ClientHandler.channelRead0(Handlers.scala:53)
        at org.labrad.manager.ClientHandler.channelRead0(Handlers.scala:35)
        at io.netty.channel.SimpleChannelInboundHandler.channelRead(SimpleChannelInboundHandler.java:105)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:334)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:326)
        at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:102)
        at io.netty.handler.codec.MessageToMessageCodec.channelRead(MessageToMessageCodec.java:111)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:334)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:326)
        at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:293)
        at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:267)
        at io.netty.handler.codec.ByteToMessageCodec.channelRead(ByteToMessageCodec.java:103)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:334)
        at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:326)
        at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1320)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
        at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:334)
        at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:905)
        at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:123)
        at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:563)
        at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:504)
        at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:418)
        at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:390)
        at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:742)
        at java.lang.Thread.run(Unknown Source)

This is likely the problematic function: https://github.com/labrad/scalabrad/blob/master/core/src/main/scala/org/labrad/data/Data.scala#L836-L859

maffoo commented 2 years ago

Unit annotations on server methods are meant to specify a single unit that the server wants, and then the labrad manager transparently converts the units given by the client to the units specified by the server, which fails if the units are not compatible. Specifying multiple incompatible units for a parameter on a server method was not something we intended to support. We could add support for this, but my suggestion would be instead to create separate server settings, e.g. have a set_power setting that takes v[dBm] and a separate set_amplitude setting that takes v[mV], or something like that.

I guess the options are:

  1. Update the labrad manager to allow configuring settings with multiple incompatible units, and then convert to whichever unit is compatible with a value in a given request.
  2. Continue to support only one unit for a given value, but update the @setting decorator and the labrad manager to fail if a server tries to configure a setting with multiple incompatible units for a given parameter. This would push the errors to server startup time rather than when handling a given request, and allow us to make the error messages more clear.

I should also note that I think interpreting v type annotation on a parameter as accepting any unit was a mistake. Instead I think v should mean a "bare" float with no units, and we should have a separate annotation like v[?] to indicate that any unit is allowed. This would not be a backward-compatible change, so we'd need some way to opt-in to the new meaning of v.

xiaoyuejin commented 2 years ago

Hi Matthew,

Nice to see you here again!

Regarding the options, both options are OK to work with.

Personally, I feel option 2 is the more logical one, but it seems too strict to me. I mean, if input like ['s', 'i', 'b'] is allowed, why not allow ['v[dBm]', 'v[mV]']? Indeed, there is risk of potential errors, but it is nothing that we don't already have.

About the unitless float, maybe you could consider using v[]? But you would still need to allow 'v' to be accepted for those old servers. Btw, right now using 'v[]' as an input tag will generate a strange error that I can't understand.

Compared to options 1/2, the more severe problem is the lack of documentation. Option 1 or 2, or even no change to current manager are all workable with, as long as there exists corresponding wiki page. A sentence like "Specifying multiple incompatible units for a parameter on a server method is not supported." could save one quite a few hours if not a few days. There are people (myself included) who are more than happy to improve the labrad wiki pages.