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

mguijarr · 2024-05-26T21:31:12Z

… default value to characterize the value

Solves issue #1193 (and #1178 ?)

mguijarr · 2024-05-26T21:32:11Z

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

tacaswell · 2024-05-29T01:15:07Z

ophyd/signal.py

@@ -95,7 +102,7 @@ def __init__(
        self,
        *,
        name,
-        value=0.0,
+        value=DEFAULT_EPICSSIGNAL_VALUE,


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

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 ?

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.

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

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

Tell me what you prefer

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 · 2024-05-29T01:15:14Z

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 · 2024-06-04T13:39:42Z

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 · 2024-06-04T17:51:27Z

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 · 2024-06-05T14:56:18Z

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 · 2024-06-17T10:59:54Z

No news ; is there anything wrong with the proposal?

mguijarr · 2024-07-02T15:35:44Z

@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 · 2024-07-02T16:25:08Z

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

wakonig · 2024-10-11T08:14:53Z

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

mguijarr · 2024-10-28T14:37:23Z

Hi again... I am reviving this discussion, I really need the added functionality so can we find an agreement on this?

tacaswell · 2024-10-29T02:21:24Z

Sorry I never found you again at NOBUGs!

ophyd/signal.py

tacaswell · 2024-10-29T02:35:09Z

ophyd/signal.py

@@ -95,7 +104,7 @@ def __init__(
        self,
        *,
        name,
-        value=0.0,
+        value=NumericValueInfo(float, (), 0.0),


Rather than merging the expected info and the default value, can we add a new key(s) of expected_dtype_and_shape or dtype and shape? In __init__ it seems reasonable to do a cast, but in describe I wonder if we should be checking if the value we have in had is actually the correct dtype/shape or not?

In my original proposal I didn't want to add more keyword args, but yeah sure - I introduced dtype and shape in latest version.

For the check in describe: I didn't change anything yet

tacaswell · 2024-10-29T02:37:03Z

ophyd/tests/test_signal.py

+
+    original = Signal(name="original")
+    original_desc = original.describe()["original"]
+    assert original_desc["dtype"] == "float64"


This is out of the schema for things that can be in descriptors: https://github.com/bluesky/event-model/blob/a1fdaec0c45b89e513d0be9dba0bfc453982e5bb/src/event_model/schemas/event_descriptor.json#L51-L62

Ok so newest version has dtype and dtype_numpy

tacaswell · 2024-10-29T02:44:10Z

The more-complete dtype information should be put in dtype_numpy. This going in everywhere at the bottom would probably be a big help (as we have been stuffing it in ad-hoc at NSLS-II).

https://github.com/bluesky/event-model/blob/a1fdaec0c45b89e513d0be9dba0bfc453982e5bb/src/event_model/schemas/event_descriptor.json#L63-L91

mguijarr · 2024-10-29T16:52:55Z

I just pushed some code, to comply with your comments. Let's see if tests pass

tacaswell · 2024-10-29T19:04:56Z

I don't have a good theory as to why we are seeing the pytest issues on this PR. Can you try rebasing on current main?

… to characterize the value

…=float, shape=scalar Initializing with a 'simple' value will make 'describe()' use inference to determine dtype and shape at runtime

tacaswell reviewed May 29, 2024

View reviewed changes

mguijarr force-pushed the signal_valueinfo branch from 605ab8e to f1849e0 Compare June 4, 2024 13:40

mguijarr force-pushed the signal_valueinfo branch 10 times, most recently from 6c7a836 to c2be5d3 Compare June 5, 2024 14:31

mguijarr marked this pull request as draft July 9, 2024 09:09

mguijarr mentioned this pull request Jul 23, 2024

Suggestion: add dims to signal descriptor #1203

Open

mguijarr force-pushed the signal_valueinfo branch from c2be5d3 to fd75ea7 Compare October 25, 2024 08:13

tacaswell reviewed Oct 29, 2024

View reviewed changes

ophyd/signal.py Outdated Show resolved Hide resolved

tacaswell reviewed Oct 29, 2024

View reviewed changes

mguijarr force-pushed the signal_valueinfo branch 2 times, most recently from dc884b2 to 3a3cdb0 Compare October 29, 2024 16:52

mguijarr force-pushed the signal_valueinfo branch 6 times, most recently from 051d146 to 4634087 Compare October 31, 2024 13:02

Mathias Guijarro added 3 commits October 31, 2024 15:51

signal: add dtype, shape keyword arguments to constructor, to be able…

35f7b22

… to characterize the value

test: Signal without value and dtype will take default value=0, dtype…

f94d636

…=float, shape=scalar Initializing with a 'simple' value will make 'describe()' use inference to determine dtype and shape at runtime

fix: set default shape in NDDerivedSignal

392b3ad

mguijarr force-pushed the signal_valueinfo branch from 4634087 to 09540a7 Compare October 31, 2024 14:51

Mathias Guijarro added 2 commits October 31, 2024 16:03

test: add tests for different dtype, shape and Signals

f861b5d

fix: data shape returns a tuple

65c5c8a

mguijarr force-pushed the signal_valueinfo branch from 09540a7 to 65c5c8a Compare October 31, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mguijarr commented May 26, 2024

mguijarr commented May 26, 2024

tacaswell May 29, 2024

mguijarr Jun 4, 2024

tacaswell Jun 4, 2024

mguijarr Jun 4, 2024 •

edited

Loading

mguijarr Jun 4, 2024

tacaswell Jun 4, 2024

tacaswell commented May 29, 2024

mguijarr commented Jun 4, 2024

prjemian commented Jun 4, 2024

mguijarr commented Jun 5, 2024 •

edited

Loading

mguijarr commented Jun 17, 2024

mguijarr commented Jul 2, 2024

prjemian commented Jul 2, 2024

wakonig commented Oct 11, 2024

mguijarr commented Oct 28, 2024

tacaswell commented Oct 29, 2024

tacaswell Oct 29, 2024

mguijarr Oct 29, 2024

tacaswell Oct 29, 2024

mguijarr Oct 29, 2024

tacaswell commented Oct 29, 2024

mguijarr commented Oct 29, 2024

tacaswell commented Oct 29, 2024

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

Are you sure you want to change the base?

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

Conversation

mguijarr commented May 26, 2024

mguijarr commented May 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mguijarr Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented May 29, 2024

mguijarr commented Jun 4, 2024

prjemian commented Jun 4, 2024

mguijarr commented Jun 5, 2024 • edited Loading

mguijarr commented Jun 17, 2024

mguijarr commented Jul 2, 2024

prjemian commented Jul 2, 2024

wakonig commented Oct 11, 2024

mguijarr commented Oct 28, 2024

tacaswell commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Oct 29, 2024

mguijarr commented Oct 29, 2024

tacaswell commented Oct 29, 2024

mguijarr Jun 4, 2024 •

edited

Loading

mguijarr commented Jun 5, 2024 •

edited

Loading