Add functions for saving device #18

olliesilvester · 2023-09-20T13:42:09Z

For the first part of #16. Adds a set of functions used to build save plans of a device.

src/ophyd_async/core/_device/device_save_loader.py

coretl · 2023-09-21T09:45:22Z

src/ophyd_async/core/_device/device_save_loader.py

+    for attr_name, attr in device.children():
+        dot = ""
+        # Place a dot inbetween the upper and lower class.
+        # Don't do this for highest level class.
+        if path_prefix:
+            dot = "."
+        dot_path = f"{path_prefix}{dot}{attr_name}"
+        if type(attr) is SignalRW:
+            signalRWs[dot_path] = attr
+        get_signal_RWs_from_device(attr, path_prefix=dot_path)
+    return signalRWs


Using a mutable default arg will cause problems:
https://web.archive.org/web/20200221224620id_/http://effbot.org/zone/default-values.htm

I suggest you return a dict and update with the child devices.

Suggested change

for attr_name, attr in device.children():

dot = ""

# Place a dot inbetween the upper and lower class.

# Don't do this for highest level class.

if path_prefix:

dot = "."

dot_path = f"{path_prefix}{dot}{attr_name}"

if type(attr) is SignalRW:

signalRWs[dot_path] = attr

get_signal_RWs_from_device(attr, path_prefix=dot_path)

return signalRWs

signals: Dict[str, SignalRW] = {}

for attr_name, attr in device.children():

dot_path = f"{path_prefix}{attr_name}"

if isinstance(attr, SignalRW):

signals[dot_path] = attr

attr_signals = get_signal_RWs_from_device(attr, path_prefix = dot_path + ".")

signals.update(attr_signals)

return signals

coretl · 2023-09-21T09:46:47Z

src/ophyd_async/core/_device/device_save_loader.py

+from ophyd_async.core import Device, SignalRW
+
+
+def get_signal_RWs_from_device(


I think PEP8 doesn't like uppercase in functions names. How about walk_rw_signals(device, path_prefix) instead?

src/ophyd_async/core/_device/device_save_loader.py

coretl · 2023-09-21T09:48:44Z

src/ophyd_async/core/_device/device_save_loader.py

+                        value[inner_key] = inner_value.tolist()
+            # Convert enums to their values
+            elif isinstance(signal_values[index], Enum):
+                signal_values[index] = value.value


Here we should assert isinstance(str) as all Enums should be instances of strings

coretl · 2023-09-21T09:52:08Z

src/ophyd_async/core/_device/device_save_loader.py

+    return signalRWs
+
+
+def save_device(device: Device, savename: str, ignore: List[str] = []):


I would put this phase sorting and the ignore list in a device specific plan, and also move the yaml saving to another function:

Suggested change

def save_device(device: Device, savename: str, ignore: List[str] = []):

def save_signals(signals: List[Dict[str, SignalRW]], ignore: Sequence[str] = ()) -> List[Dict[str, Any]]:

olliesilvester · 2023-09-22T11:18:55Z

Now you make a plan using a series of utility functions in device_save_loader.py. I've put some examples in device_save_loader_test.py
In general a save plan should:

Get the RW signals with walk_rw_signals()
Locate the values of each of these with get_signal_values(), with an optional parameter to ignore specific signals
Optionally sort the values into separate phases with a device specific function
Save these values to a yaml file with save_to_yaml()

coretl · 2023-09-22T15:56:49Z

src/ophyd_async/core/_device/device_save_loader.py

+    if ignore is None:
+        ignore = [""]
+
+    values = yield Msg("locate", *signals.values())


This will locate signals in the ignore list, so probably nest to exclude them. Also we want to make sure the values are None in case we want to use a save file as a source of ignores for the future.

How about something like:

ignore = ignore or [] selected_signals = {key: signal for key, signal in signals.items() if key not in ignore} selected_values = yield Msg("locate", *selected_signals.values()) named_values = {key, value["setpoint"] for key, value in zip(selected_signals, selected_values)} # Ignored values place in with value None so we know which ones were ignored named_values.update({key: None for key in ignore})

coretl · 2023-09-22T15:57:35Z

src/ophyd_async/core/_device/device_save_loader.py

+
+
+def walk_rw_signals(
+    device: Device, path_prefix: Optional[str] = None


Suggested change

device: Device, path_prefix: Optional[str] = None

device: Device, path_prefix: str = ""

Strings aren't mutable so fine to use the empty string as a default

coretl · 2023-09-22T16:03:00Z

src/ophyd_async/core/_device/device_save_loader.py

+        phase_outputs.append(phase)
+
+    with open(save_path, "w") as file:
+        yaml.dump(phase_outputs, file)


I think you can do the numpy formatting here with a custom yaml dumper, at least you can in ruamel.yaml, not sure about pyyaml. That might be cleaner than preprocessing the structures by hand...

This makes sense - Do you think it's worth doing this for Enums as well? I don't think this can be done in the same way since yaml.add_representer doesn't appear to work for subclasses

Maybe? Although if we make sure we define MyEnum(str, Enum) then it should just serialize as it is?

If we do this, it will automatically serialize it as, eg

my_enum: !!python/object/apply:test_device_save_loader.EnumTest - val1

and can then reads this value as a string when doing yaml.load(). Otherwise I can keep the logic which converts the enum object to a simple string when serialising.

Ok, we want to make sure the yaml contains a plain string, so I guess we do need to do that pre-processing

OK, this is back now

coretl · 2023-10-09T14:02:22Z

tests/core/_device/test_device_save_loader.py

+
+    RE(save_my_device())
+
+    yaml.add_constructor("tag:!numpy.ndarray", ndarray_constructor)


Does the YAML file have these tags in it?

If it does, then we should use a dumper that allows transformation of the data by type (which may fix the enums too):
https://stackoverflow.com/questions/65866546/how-to-make-ruamel-yaml-dump-formatted-numpy-arrays-as-simple-list

jsouter · 2023-10-13T12:55:39Z

I have made a PR from my fork. Still using PyYAML - ruamel.yaml was more difficult to get pretty formatting out of.
#27
Now using a custom Dumper subclass that checks if the data is an Enum and if so represents its value member. A custom representer was also added for np.ndarrays to cast it to a list when representing; if needed this logic could be moved into the OphydDumper class's represent_data overload as well.

coretl · 2023-10-23T15:44:32Z

Superseded by #36

olliesilvester commented Sep 20, 2023

View reviewed changes

src/ophyd_async/core/_device/device_save_loader.py Outdated Show resolved Hide resolved

olliesilvester requested a review from coretl September 20, 2023 13:43

coretl reviewed Sep 21, 2023

View reviewed changes

olliesilvester added 6 commits September 22, 2023 12:12

add initial save and load methods

c3e9120

Move save to plan, make signalRW locatable, add save test

18f8f74

add get_setpoint to signal backends

3b650d4

Rework get_signal_RWS_from_device

ac2d036

add get_signal_values

568beb6

split up save plan into composite functions

30346d5

olliesilvester force-pushed the add_save_load branch from 5b98f9f to 30346d5 Compare September 22, 2023 11:13

olliesilvester requested a review from coretl September 22, 2023 11:19

olliesilvester changed the title ~~Add save_device plan~~ Add functions for saving device Sep 22, 2023

coretl reviewed Sep 22, 2023

View reviewed changes

olliesilvester added 2 commits September 26, 2023 10:29

Don't locate ignore values and save them as None

6bceb58

now do custom formatting for numpy arrays

5fbb60a

olliesilvester force-pushed the add_save_load branch from ab8e478 to 5fbb60a Compare September 26, 2023 12:32

olliesilvester added 5 commits September 26, 2023 13:36

linting

8642d63

Ophyd enums should inherit from string

43cd847

save plan no longer handles enums explicitly

1d0c620

revert accidentally removed test

3130842

Readded enum preprocessing

9e631af

olliesilvester requested a review from coretl October 3, 2023 08:16

coretl reviewed Oct 9, 2023

View reviewed changes

coretl mentioned this pull request Oct 23, 2023

Add save load #36

Merged

coretl closed this Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functions for saving device #18

Add functions for saving device #18

olliesilvester commented Sep 20, 2023 •

edited

Loading

coretl Sep 21, 2023

coretl Sep 21, 2023

coretl Sep 21, 2023

coretl Sep 21, 2023

olliesilvester commented Sep 22, 2023

coretl Sep 22, 2023 •

edited

Loading

coretl Sep 22, 2023

coretl Sep 22, 2023

olliesilvester Sep 26, 2023

coretl Sep 26, 2023

olliesilvester Sep 29, 2023 •

edited

Loading

coretl Sep 29, 2023

olliesilvester Sep 29, 2023

coretl Oct 9, 2023

coretl Oct 9, 2023

jsouter commented Oct 13, 2023

coretl commented Oct 23, 2023

		from ophyd_async.core import Device, SignalRW


		def get_signal_RWs_from_device(

		return signalRWs


		def save_device(device: Device, savename: str, ignore: List[str] = []):

	def save_device(device: Device, savename: str, ignore: List[str] = []):
	def save_signals(signals: List[Dict[str, SignalRW]], ignore: Sequence[str] = ()) -> List[Dict[str, Any]]:



		def walk_rw_signals(
		device: Device, path_prefix: Optional[str] = None

	device: Device, path_prefix: Optional[str] = None
	device: Device, path_prefix: str = ""


		RE(save_my_device())

		yaml.add_constructor("tag:!numpy.ndarray", ndarray_constructor)

Add functions for saving device #18

Add functions for saving device #18

Conversation

olliesilvester commented Sep 20, 2023 • edited Loading

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

olliesilvester commented Sep 22, 2023

coretl Sep 22, 2023 • edited Loading

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

Choose a reason for hiding this comment

olliesilvester Sep 29, 2023 • edited Loading

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

Choose a reason for hiding this comment

jsouter commented Oct 13, 2023

coretl commented Oct 23, 2023

olliesilvester commented Sep 20, 2023 •

edited

Loading

coretl Sep 22, 2023 •

edited

Loading

olliesilvester Sep 29, 2023 •

edited

Loading