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-11-04T14:42:33Z

@tacaswell sorry for the delay... I struggled a bit with the changes (this is why test didn't pass last time).

In fact I asked me a lot of questions, finally I think I get it all right.

For the review: maybe the easiest is to have a look to the new test (test_signal_dtype_shape_info), because it shows the expected behavior. So if you agree with this, then you can maybe look at changes in details ?

What I find relevant to know:

I added keyword arguments dtype and shape which defaults to None
The default value is 0.0, but I identify it with a DefaultFloat class
- this is to distinguish if user provided a value, or left default
- indeed, I need to know this in some circumstances like a string signal without specific default passed by the user
  - in this case, I do not initialize with 0.0 but with UNSET_VALUE (previously DEFAULT_EPICSSIGNAL_VALUE, which is still there for compatibility)
the UNSET_VALUE has a __repr__, so we can see it while debugging more easily
if a default value is passed, there is a check to validate if it corresponds to the dtype (or shape)
- it was not so obvious to allow numpy.int64 (default for ints) to an unsigned int for example, numpy can be tricky!
- there is no cast, but TypeError is raised if it does not correspond
I removed a weird __copy__ dunder
- I could not see why it was like this, no specific test neither
- with the original __copy__ one test was not passing, because it recreates the object with different constructor arguments
in describe(), if the dtype and shape are specified it is used for determining infos instead of inferring from data
- otherwise it is like before
in describe(), if numpy dtype is specified it goes to dtype_numpy as you required
- however, if it is not available the key is also not set in the dictionary (allows old tests to pass)
I added .source_name as a property, in order to make describe() more generic
I changed data_shape to return a tuple (because, it is really a shape), however it is returned as a list by describe() (like now) to make it JSON compliant

mguijarr · 2024-11-04T14:46:09Z

Other remark:

would it be possible to get rid of integer for simple dtype in describe() ?
- now we have dtype_numpy I don't see why we would need this
- it would simplify some code
would it make more sense to not initialize value to 0.0 ?
- I would prefer UNSET_VALUE

tacaswell · 2024-11-04T21:53:36Z

would it be possible to get rid of integer for simple dtype in describe() ?

Maybe we could get the RE to look at numpy_dtype and infer the correct dtype, however in the document model dtype is required but numpy_dytpe is optional (https://github.com/bluesky/event-model/blob/a1fdaec0c45b89e513d0be9dba0bfc453982e5bb/src/event_model/schemas/event_descriptor.json#L145-L149) so I think it makes sense to always provide both from ophyd.

would it make more sense to not initialize value to 0.0 ?

That seems fine as 0.0 is not a valid value for a great many dtypes!

tacaswell · 2024-11-04T21:55:13Z

ophyd/ophydobj.py

@@ -588,14 +588,14 @@ def _repr_info(self):
        if self._parent is not None:
            yield ("parent", self.parent.name)

-    def __copy__(self):


Why did you remove the custom __copy__?

I said in the comment above:

I removed a weird copy dunder

with the original copy one test was not passing, because it recreates the object with different constructor arguments

I could not see why it was like this, no specific test neither

But, with my newest code the failing test works again. So I re-enabled __copy__, no prob.

(however, I still don't understand what it is for)

ophyd/signal.py

… to characterize the value

mguijarr · 2024-11-05T12:46:04Z

@tacaswell Here we are. Should be ready to merge now ?

mguijarr · 2024-11-05T12:50:01Z

I think it makes sense to always provide both from ophyd.

Just to be precise: current code I sent, do not always provide dtype_numpy ; indeed, the document model check do not understand empty string as a valid dtype_numpy. So, in this case (if dtype_numpy is unknown), it is not in the dict returned by describe() at all (otherwise the verification logic has to be changed)

About replacing 0.0 default value with UNSET_VALUE:

That seems fine as 0.0 is not a valid value for a great many dtypes!

Ok. I will make another MR, then. It is not urgent for me.

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 11 times, most recently from 75c4f6e to 7e33cc1 Compare November 4, 2024 14:08

mguijarr force-pushed the signal_valueinfo branch from 7e33cc1 to cf6b4a5 Compare November 4, 2024 14:43

tacaswell reviewed Nov 4, 2024

View reviewed changes

ophyd/signal.py Outdated Show resolved Hide resolved

mguijarr force-pushed the signal_valueinfo branch from 0012943 to 4537842 Compare November 5, 2024 09:54

Mathias Guijarro added 4 commits November 5, 2024 11:18

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

da948eb

… to characterize the value

fix: set default shape in NDDerivedSignal

1038ac8

test: add tests for different dtype, shape and Signals

e93622e

fix: data shape returns a tuple

db157c5

mguijarr force-pushed the signal_valueinfo branch from 4537842 to db157c5 Compare November 5, 2024 10:19

tacaswell marked this pull request as ready for review November 12, 2024 16:57

tacaswell approved these changes Nov 12, 2024

View reviewed changes

tacaswell merged commit ee7ab9b into bluesky:master Nov 12, 2024
10 checks passed

This was referenced Nov 12, 2024

Option to force "number" rather than "integer" in Signal #1178

Open

Allow to specify dtype and shape without type inference for Signal description #1193

Closed

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 Nov 4, 2024

mguijarr commented Nov 4, 2024

tacaswell commented Nov 4, 2024

tacaswell Nov 4, 2024

mguijarr Nov 5, 2024

mguijarr commented Nov 5, 2024

mguijarr commented Nov 5, 2024

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

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 Nov 4, 2024

mguijarr commented Nov 4, 2024

tacaswell commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mguijarr commented Nov 5, 2024

mguijarr commented Nov 5, 2024

mguijarr Jun 4, 2024 •

edited

Loading

mguijarr commented Jun 5, 2024 •

edited

Loading