signal: init value can be a ValueInfo structure with dtype, shape and…

[mguijarr]

… default value to characterize the value

Solves issue #1193 (and #1178 ?)

[mguijarr]

Please comment , if you agree with the proposed change I will add a test.

[tacaswell]

If we move this here I suspect we will have to hoist the logic to handle this from EpicsSignal up to the base class.

[mguijarr]

well, it looks like DEFAULT_EPICSSIGNAL_VALUE is more used as a sentinel value to indicate if a default has been provided or not - it is currently already used in Signal class, see line 475:

if self._readback is DEFAULT_EPICSSIGNAL_VALUE:
            val = self.get()

or derived classes which do not have an explicit relation with EPICS (i.e not deriving from EpicsSignalBase).

I propose to rename it, then ? Like UNSET_VALUE or UNSPECIFIED_VALUE ?

It seems None is considered as a potential ok value. Is it the case ? Otherwise we could initialize with None.

In describe(), I think the key point is to really read the value if it is not specified. Any way to do it like this is fine for me.

What do you think ?

[tacaswell]

Oh, if we are already using it is the base class than I'm 50/50 on adding an alias (the hassle of anyone downstream using the old name needing to update is not worth it to remove the old name) vs just using it.

The concern I still have is that in Signal.get we just return self._readback which means that there is a chance of returning DEFAULT_EPICSSIGNAL_VALUE to the user (which we definitely should not do). The minimal fix is probably to raise in Signal.get if the value has never been set.

[mguijarr]

Wouldn't it be better to initialize to None and to return None if it has not been set? I am very new to Ophyd, so I do not know the logic/philosophy behind reading signals and why DEFAULT_EPICSSIGNAL_VALUE was introduced at first place

[mguijarr]

Otherwise I can make an alias and raise in get() as you suggest, no problem 👍🏻

Tell me what you prefer

[tacaswell]

tl;dr: I would prefer the alias + raise in get path as the minimal change.

pyepics (the default wrapping we use to talk channel access over in Python) returns None when it fails so defaulting to None means we could not tell the difference between "failed to read" vs "never read". The logic was to have a unique sentinel that would never come to use from pyepics and would never be passed to us by the user so we could tell if we had ever been told a value for this signal. Basically detect the start up transient and allows lazy connection (each PV is its own connection dance with a server and if we never read it then it is better to never try to connect).

Going with a custom default object also helps makes clear that it is "default but invalid" where as None is frequently "default and valid" or "placeholder to tell the system to do the default thing" and I think that is an important distinction here.

I'm a bit fuzzy remember choices we made probably 8 years ago now, but I am biased to trusted my previous self 🤣

[tacaswell]

Interesting idea!

I have some concerns that this will box us out from ever fixing EpicsSignal to trust the metadata that comes across on the channel rather than infering the dtype from the values, but I think that if the rules go 1. user passed 2. Channel provided 3. inferred would be ok.

We have been using dtype_str for providing higher-fidelitiy data type information (see discussion in bluesky/event-model#215 despite not getting that PR merged, we have started to use it in-practice in tiled) so we probably should use a different name internally.

I lean +.75 on this being a good idea .

[mguijarr]

I have some concerns that this will box us out from ever fixing EpicsSignal to trust the metadata that comes across on the channel rather than infering the dtype from the values, but I think that if the rules go 1. user passed 2. Channel provided 3. inferred would be ok.

I agree with this.

We have been using dtype_str for providing higher-fidelitiy data type information (see discussion in bluesky/event-model#215 despite not getting that PR merged, we have started to use it in-practice in tiled) so we probably should use a different name internally.

Ok I renamed shape and dtype vars to _value_shape and _value_dtype_str.

[prjemian]

The value of DEFAULT_EPICSSIGNAL_VALUE should be set such that (lazy) programmatic use does not rely on the actual value of the default. Random value should be assigned to require use of the symbol, such as one of these:

DEFAULT_EPICSSIGNAL_VALUE = object()
DEFAULT_EPICSSIGNAL_VALUE = uuid.uuid4()

[mguijarr]

Hi @tacaswell , @prjemian 😄 This is ready for me. I don't know why doc would not build... ?

I added specific tests for the feature. In addition to what we discussed above, I renamed ValueInfo to NumericValueInfo because it is not adapted to strings
for example. I added source_name as a property, in order to make describe() more generic. I made that shape in describe() is a tuple rather than sometimes a list, sometimes a tuple (because, for numpy arrays, it is a tuple normally).

I would be happy to work on having EpicsSignalBase trying to specify dtype and shape from metadata but I have no idea how to do it since I am very new to EPICS, PyEpics in general. Normally it is easy to do it like @tacaswell proposes, 1. user defined , 2. EPICS metadata or 3. inference if there is nothing else... Maybe in another MR ?

[mguijarr]

No news ; is there anything wrong with the proposal?

[mguijarr]

@prjemian @tacaswell I apologize in advance if you're on holidays or too busy. I really need to have proper type info in Ophyd, sorry to insist - if you think this proposal can be improved do not hesitate to tell me, or if I have to do something to make it approved ?

[prjemian]

I'm OK with the part on which I commented. I'll leave it to Tom for the PR review.

[wakonig]

Hi @tacaswell! What's the status on this one? It would help us a lot if we could get this one merged soon. Thanks!