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

mguijarr commented 6 months ago

… default value to characterize the value

Solves issue #1193 (and #1178 ?)

mguijarr commented 6 months ago

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

tacaswell commented 6 months ago

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 https://github.com/bluesky/event-model/pull/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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

No news ; is there anything wrong with the proposal?

mguijarr commented 4 months ago

@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 commented 4 months ago

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

wakonig commented 1 month ago

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 commented 1 month ago

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

tacaswell commented 1 month ago

Sorry I never found you again at NOBUGs!

tacaswell commented 1 month ago

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 commented 1 month ago

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

tacaswell commented 1 month ago

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?

mguijarr commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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!

mguijarr commented 3 weeks ago

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

mguijarr commented 3 weeks ago

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.

bluesky / ophyd

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