Rebase add_save_load on add-detector-logic #31

jsouter · 2023-10-16T10:26:08Z

Rebased on add-detector-logic, tests are passing on my end but let me know if things still need moving around

coretl · 2023-10-16T10:35:17Z

Please can you fix linting. @rosesyrett when that is fixed shall we merge?

jsouter · 2023-10-16T12:52:24Z

Please can you fix linting. @rosesyrett when that is fixed shall we merge?

Linting is fixed now

rosesyrett

Looks good, I like how only SignalRW's are saved, that's very neat. I have a few comments about docstrings and a few about the tests, as I use tests to work out how to use a new bit of code so to me it's important they're readable and clear.
Very nearly there.

rosesyrett · 2023-10-16T13:08:14Z

src/ophyd_async/core/signal_backend.py

+    async def get_setpoint(self) -> T:
+        """The current setpoint"""


without going into bluesky.protocols and looking at the definition of a Location protocol and its comments for setpoint and readback, this docstring is a little confusing. Could we perhaps replace it with:

Suggested change

async def get_setpoint(self) -> T:

"""The current setpoint"""

async def get_setpoint(self) -> T:

"""The point that a signal was requested to move to."""

rosesyrett · 2023-10-16T13:10:37Z

src/ophyd_async/core/signal_backend.py

    async def get_value(self) -> T:
        """The current value"""


In line with making setpoint and readback slightly less confusing, could we replace this with:

Suggested change

async def get_value(self) -> T:

"""The current value"""

async def get_value(self) -> T:

"""The readback value of the signal."""

rosesyrett · 2023-10-16T13:12:49Z

src/ophyd_async/core/sim_signal_backend.py

+    async def get_setpoint(self) -> T:
+        return self.converter.value(self._value)


is it worth making this just call get_value, and adding a comment that for a simsignalbackend these will always be the same?

i.e.:

Suggested change

async def get_setpoint(self) -> T:

return self.converter.value(self._value)

async def get_setpoint(self) -> T:

"""For a simulated backend, the setpoint and readback values are the same."""

return await self.get_value()

rosesyrett · 2023-10-16T13:19:35Z

tests/core/test_device_save_loader.py

+
+
+async def test_enum_yaml_formatting(tmp_path):
+    enums = [EnumTest(EnumTest.VAL1), EnumTest(EnumTest.VAL2)]


I think it's easier to read this as:

Suggested change

enums = [EnumTest(EnumTest.VAL1), EnumTest(EnumTest.VAL2)]

enums = [EnumTest.VAL1, EnumTest.VAL2]

Ah you're right, was mistaken about how Python Enums work

rosesyrett · 2023-10-16T13:23:37Z

tests/core/test_device_save_loader.py

+    assert isinstance(enums[0], EnumTest)
+    save_to_yaml(enums, path.join(tmp_path, "test_file.yaml"))
+    with open(path.join(tmp_path, "test_file.yaml"), "r") as file:
+        yaml_content = yaml.load(file, yaml.Loader)
+        assert isinstance(yaml_content, list)
+        assert yaml_content[0] == EnumTest.VAL1
+        assert yaml_content[1] == EnumTest.VAL2


these assertions all make sense, but I'd shorten them for readability:

Suggested change

assert isinstance(enums[0], EnumTest)

save_to_yaml(enums, path.join(tmp_path, "test_file.yaml"))

with open(path.join(tmp_path, "test_file.yaml"), "r") as file:

yaml_content = yaml.load(file, yaml.Loader)

assert isinstance(yaml_content, list)

assert yaml_content[0] == EnumTest.VAL1

assert yaml_content[1] == EnumTest.VAL2

save_to_yaml(enums, path.join(tmp_path, "test_file.yaml"))

with open(path.join(tmp_path, "test_file.yaml"), "r") as file:

saved_enums = yaml.load(file, yaml.Loader)

assert saved_enums == enums

This effectively combines the assert statements you currently have on lines 71-73, and removes the assert statement on 67 since that will never raise an AssertionError unless there is something very wrong with python or someone's redefined EnumTest... Up to you if you want to rename yaml_content or not

async def test_enum_yaml_formatting(tmp_path): enums = [EnumTest.VAL1, EnumTest.VAL2] save_to_yaml(enums, path.join(tmp_path, "test_file.yaml")) with open(path.join(tmp_path, "test_file.yaml"), "r") as file: saved_enums = yaml.load(file, yaml.Loader) # check that save/load reduces from enum to str assert all(isinstance(value, str) for value in saved_enums) # check values of enums same assert saved_enums == enums

How about this? I'd like to have something that shows that reduction to a string when saving to yaml

rosesyrett · 2023-10-16T13:32:17Z

tests/core/test_device_save_loader.py

+async def test_save_device_with_phase(device_with_phases, tmp_path):
+    RE = RunEngine()
+    await device_with_phases.child1.sig1.set("string")
+    table_pv = {"VAL1": np.array([1, 1, 1, 1, 1]), "VAL2": np.array([1, 1, 1, 1, 1])}
+    await device_with_phases.child2.sig1.set(table_pv)
+
+    # Create save plan from utility functions
+    def save_my_device():
+        signalRWs = walk_rw_signals(device_with_phases)
+        values = yield from get_signal_values(signalRWs)
+        phases = device_with_phases.sort_signal_by_phase(device_with_phases, values)
+        save_to_yaml(phases, path.join(tmp_path, "test_file.yaml"))
+
+    RE(save_my_device())
+
+    with open(path.join(tmp_path, "test_file.yaml"), "r") as file:
+        yaml_content = yaml.load(file, yaml.Loader)
+        assert yaml_content[0] == {"child1.sig1": "string"}
+        assert np.array_equal(
+            yaml_content[1]["child2.sig1"]["VAL1"], np.array([1, 1, 1, 1, 1])
+        )
+        assert np.array_equal(
+            yaml_content[1]["child2.sig1"]["VAL2"], np.array([1, 1, 1, 1, 1])
+        )
+
+
+async def test_yaml_formatting_no_phase(device_with_phases, tmp_path):
+    RE = RunEngine()
+    await device_with_phases.child1.sig1.set("test_string")
+    table_pv = {"VAL1": np.array([1, 2, 3, 4, 5]), "VAL2": np.array([6, 7, 8, 9, 10])}
+    await device_with_phases.child2.sig1.set(table_pv)
+
+    # Create save plan from utility functions
+    def save_my_device():
+        signalRWs = walk_rw_signals(device_with_phases)
+        values = yield from get_signal_values(signalRWs)
+        phases = device_with_phases.sort_signal_by_phase(device_with_phases, values)
+        save_to_yaml(phases, path.join(tmp_path, "test_file.yaml"))
+
+    RE(save_my_device())
+
+    with open(path.join(tmp_path, "test_file.yaml"), "r") as file:
+        expected = """\
+- {child1.sig1: test_string}
+- child2.sig1:
+    VAL1: [1, 2, 3, 4, 5]
+    VAL2: [6, 7, 8, 9, 10]
+"""
+        assert file.read() == expected
+
+
+async def test_saved_types_with_phase(device_with_phases, tmp_path):
+    RE = RunEngine()
+    await device_with_phases.child1.sig1.set("string")
+    table_pv = {"VAL1": np.array([1, 1, 1, 1, 1]), "VAL2": np.array([1, 1, 1, 1, 1])}
+    await device_with_phases.child2.sig1.set(table_pv)
+
+    # Create save plan from utility functions
+    def save_my_device():
+        signalRWs = walk_rw_signals(device_with_phases)
+        values = yield from get_signal_values(signalRWs)
+        phases = device_with_phases.sort_signal_by_phase(device_with_phases, values)
+        save_to_yaml(phases, path.join(tmp_path, "test_file.yaml"))
+
+    RE(save_my_device())
+
+    with open(path.join(tmp_path, "test_file.yaml"), "r") as file:
+        yaml_content = yaml.load(file, yaml.Loader)
+        assert type(yaml_content[0]["child1.sig1"]) is str
+        assert type(yaml_content[1]["child2.sig1"]["VAL1"]) is list
+        assert type(yaml_content[1]["child2.sig1"]["VAL2"]) is list


not sure about the language here r.e. phases. It seems like these three tests share a lot in common, and can probably be combined into one test and still cover the same lines.

to me, these tests only differ from the previous few because they test if save_to_yaml can handle a List[Dict[str, Any]] as well as a Dict[str, Any]. Which is fine, but why the word 'phase'? Would changing 'phase' to 'list' be better in this instance?

Yep, I can reduce these tests down. I believe the idea with "phases" is that when loading values from a yaml to a Device (which is yet to be implemented) we want to load the groups of signals in a specified order, so all the phase 1 signals first, then phase 2 signals etc. There's possibly a clearer way to express this.

Yes, I suggested the word "phase" because I imagine the loading being done in phases:

def how_i_imaging_load(device: Device, phases: Sequence[Dict[str, Any]]): signals = walk_rw_signals(device) for phase in phases: for name, value in phase.items(): yield from bps.abs_set(signals[name], value, group="load", wait=False) yield from bps.wait(group="load")

I agree that device_with_phases is a bit confusing though, as the phases are in the saved set, not the device. I don't think we will be attaching the sort_signals_by_phase method to a device, so maybe just leave that free and ditch the fixture?

jsouter · 2023-10-17T08:42:56Z

I've made those changes and added in Ollie's load_device plan as load_from_yaml, with a new test.

coretl · 2023-10-17T12:14:44Z

src/ophyd_async/core/device_save_loader.py

+
+def get_signal_values(
+    signals: Dict[str, SignalRW[Any]], ignore: Optional[List[str]] = None
+) -> Union[Generator[Dict[str, Any], None, None], Dict[str, Any]]:


I think this is always a generator. @callumforrester is this right for a plan?

Suggested change

) -> Union[Generator[Dict[str, Any], None, None], Dict[str, Any]]:

) -> Generator[Msg, Sequence[Location], Dict[str, Any]]:

src/ophyd_async/core/device_save_loader.py

jsouter · 2023-10-18T08:59:27Z

Made those changes, and added a return hint for load_from_yaml (Generator[Msg, None, Sequence[Dict[str[Any]])

coretl · 2023-10-20T09:48:41Z

src/ophyd_async/core/device_save_loader.py

+    :func:`ophyd_async.core.save_to_yaml`
+    """
+    with open(save_path, "r") as file:
+        data_by_phase: Sequence[Dict[str, Any]] = yaml.full_load(file)


Can we split this function here please. load_from_yaml(save_path: str) -> Sequence[Dict[str, Any]]:, then a different plan set_signal_values(signals: Dict[str, SignalRW], values: Sequence[Dict[str, Any]]) -> Generator[Msg, None, None]: that takes signals (as provided by walk_rw_signals), and values (as provided by load_from_yaml) and does the abs_set logic you do below

Okay, have done this rebased onto the last passing commit for add-detector-logic

Custom YAML dumper to represent Enum subclasses as values

Add device_save_loader yaml formatting tests

Co-authored-by: Tom C (DLS) <[email protected]> fix Msg import in device_save_loader.py

device_save_loader linting

Add load_from_yaml to device_save_loader update test_device_save_loader.py Co-authored-by: Oliver Silvester <[email protected]>

Require sequence of dicts in save_to_yaml rebase on newer add-detector-logic commits

jsouter mentioned this pull request Oct 16, 2023

Represent numpy ndarray as list in YAML #27

Closed

jsouter force-pushed the add_save_load_rb branch 3 times, most recently from a37320c to c0e6a0a Compare October 16, 2023 12:38

rosesyrett requested changes Oct 16, 2023

View reviewed changes

jsouter force-pushed the add_save_load_rb branch 2 times, most recently from e1cd6fa to 7e13c22 Compare October 17, 2023 08:39

coretl reviewed Oct 17, 2023

View reviewed changes

jsouter force-pushed the add_save_load_rb branch from f021e9e to ad6d838 Compare October 18, 2023 08:56

coretl reviewed Oct 20, 2023

View reviewed changes

olliesilvester and others added 16 commits October 20, 2023 14:33

add initial save and load methods

2777f6e

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

d7f0736

add get_setpoint to signal backends

75d1cd4

Rework get_signal_RWS_from_device

f08c2f1

add get_signal_values

df5c0ff

split up save plan into composite functions

5f28bae

Don't locate ignore values and save them as None

bcbad08

now do custom formatting for numpy arrays

d0db038

linting

251a34d

Ophyd enums should inherit from string

2e73141

save plan no longer handles enums explicitly

eb6a41b

revert accidentally removed test

f7aab4d

Readded enum preprocessing

299e7ec

Represent numpy ndarray as list in YAML

9516f2a

Custom YAML dumper to represent Enum subclasses as values

remove unused import, use numpy typing

12b4525

Add device_save_loader yaml formatting tests

fix black on test_yaml_formatting_no_phase

51c893c

Co-authored-by: Tom C (DLS) <[email protected]> fix Msg import in device_save_loader.py

jsouter and others added 5 commits October 20, 2023 14:33

cleanup after rebasing, move device_save_loader from deleted _device

e0a309e

fix numpy typing import for flake8 isort

956f124

device_save_loader linting

implement PR comments

7970e6c

Add load_from_yaml to device_save_loader update test_device_save_loader.py Co-authored-by: Oliver Silvester <[email protected]>

linting

df97778

Change device_save_loader type hints

eb39986

Require sequence of dicts in save_to_yaml rebase on newer add-detector-logic commits

jsouter force-pushed the add_save_load_rb branch from 615bca5 to eb39986 Compare October 20, 2023 13:36

rosesyrett deleted the branch bluesky:add-detector-logic October 20, 2023 15:29

rosesyrett closed this Oct 20, 2023

coretl mentioned this pull request Oct 23, 2023

Add save load #36

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebase add_save_load on add-detector-logic #31

Rebase add_save_load on add-detector-logic #31

jsouter commented Oct 16, 2023

coretl commented Oct 16, 2023

jsouter commented Oct 16, 2023

rosesyrett left a comment

rosesyrett Oct 16, 2023

rosesyrett Oct 16, 2023

rosesyrett Oct 16, 2023

rosesyrett Oct 16, 2023

jsouter Oct 16, 2023

rosesyrett Oct 16, 2023

jsouter Oct 16, 2023

rosesyrett Oct 16, 2023

jsouter Oct 16, 2023

coretl Oct 16, 2023

jsouter commented Oct 17, 2023

coretl Oct 17, 2023

jsouter commented Oct 18, 2023

coretl Oct 20, 2023

jsouter Oct 20, 2023

		async def get_setpoint(self) -> T:
		"""The current setpoint"""

		async def get_setpoint(self) -> T:
		return self.converter.value(self._value)



		async def test_enum_yaml_formatting(tmp_path):
		enums = [EnumTest(EnumTest.VAL1), EnumTest(EnumTest.VAL2)]

	enums = [EnumTest(EnumTest.VAL1), EnumTest(EnumTest.VAL2)]
	enums = [EnumTest.VAL1, EnumTest.VAL2]

	) -> Union[Generator[Dict[str, Any], None, None], Dict[str, Any]]:
	) -> Generator[Msg, Sequence[Location], Dict[str, Any]]:

Rebase add_save_load on add-detector-logic #31

Rebase add_save_load on add-detector-logic #31

Conversation

jsouter commented Oct 16, 2023

coretl commented Oct 16, 2023

jsouter commented Oct 16, 2023

rosesyrett left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsouter commented Oct 17, 2023

Choose a reason for hiding this comment

jsouter commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment