Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

282-standardise-handling-of-read-config-and-hinted-signals-for-standarddetector #468

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ZohebShaikh
Copy link
Contributor

No description provided.

@ZohebShaikh ZohebShaikh force-pushed the 282-standardise-handling-of-read-config-and-hinted-signals-for-standarddetector branch from 06c7a13 to b137fea Compare July 18, 2024 15:55
@ZohebShaikh ZohebShaikh changed the title initial commit 282-standardise-handling-of-read-config-and-hinted-signals-for-standarddetector Jul 22, 2024
@ZohebShaikh ZohebShaikh marked this pull request as ready for review July 22, 2024 12:09
@ZohebShaikh ZohebShaikh requested a review from coretl July 22, 2024 12:09
src/ophyd_async/epics/areadetector/writers/hdf_writer.py Outdated Show resolved Hide resolved
src/ophyd_async/epics/areadetector/writers/hdf_writer.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/nd_attributes.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/nd_attributes.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/_nd_attributes.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/_nd_attributes.py Outdated Show resolved Hide resolved
src/ophyd_async/plan_stubs/_nd_attributes.py Outdated Show resolved Hide resolved
@ZohebShaikh ZohebShaikh requested a review from coretl July 24, 2024 12:13
@@ -29,13 +31,14 @@ def __init__(
path_provider: PathProvider,
name_provider: NameProvider,
shape_provider: ShapeProvider,
**scalar_datasets_paths: str,
plugins: Sequence[NDArrayBaseIO] | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could: be positional args

Suggested change
plugins: Sequence[NDArrayBaseIO] | None = None,
*plugins: NDArrayBaseIO,

Comment on lines 96 to 114
try:
for plugin in self._plugins:
tree = ET.parse((await plugin.nd_attributes_file.get_value()))
root = tree.getroot()
for child in root:
datakey = child.attrib["name"]
self._datasets.append(
HDFDataset(
datakey,
f"/entry/instrument/NDAttributes/{datakey}",
(),
convert_ad_dtype_to_np(
ADBaseDataType((child.attrib.get("datatype", None)))
),
multiplier,
)
)
except ET.ParseError:
raise ValueError("Error parsing XML")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more, please can we

  • Only parse plugins with stuff that looks like XML
  • We can then ditch the error catching
Suggested change
try:
for plugin in self._plugins:
tree = ET.parse((await plugin.nd_attributes_file.get_value()))
root = tree.getroot()
for child in root:
datakey = child.attrib["name"]
self._datasets.append(
HDFDataset(
datakey,
f"/entry/instrument/NDAttributes/{datakey}",
(),
convert_ad_dtype_to_np(
ADBaseDataType((child.attrib.get("datatype", None)))
),
multiplier,
)
)
except ET.ParseError:
raise ValueError("Error parsing XML")
for plugin in self._plugins:
maybe_xml = await plugin.nd_attributes_file.get_value()
# This is the check that ADCore does to see if it is an XML string
# rather than a filename to parse
if "<Attributes>" in maybe_xml:
root = ET.parse(maybe_xml).getroot()
for child in root:
datakey = child.attrib["name"]
self._datasets.append(
HDFDataset(
datakey,
f"/entry/instrument/NDAttributes/{datakey}",
(),
convert_ad_dtype_to_np(
ADBaseDataType((child.attrib.get("datatype", None)))
),
multiplier,
)
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can we have some tests for these functions? I would like to see the deleted test recreated using the new functions

Comment on lines 107 to 109
convert_ad_dtype_to_np(
ADBaseDataType((child.attrib.get("datatype", None)))
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite right, so I went reading the areaDetector code. Turns out to be more complicated than I thought.

There are 3 cases to consider:

I think we can solve with the following lookup:

_ndattribute_to_ad_datatype = {
    "FLOAT": ADBaseDataType.Float64,
    "DBR_DOUBLE: ADBaseDataType.Float64,
    ...
}
if datatype in ["STRING", "DBR_STRING"]:
    np_datatype = "s40"
elif datatype == "DBR_NATIVE":
    raise ValueError("Don't support DBR_NATIVE yet")
else:
    ad_datatype = _ndattribute_to_ad_datatype[datatype]
    np_datatype = convert_ad_dtype_to_np(datatype)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 148 to 195
async def test_stats_describe_when_plugin_configured_in_memory(RE, detectors):
for detector in detectors:
await detector.connect(mock=True)
detector.set_name(type(detector).__name__)
RE(setup_ndstats_sum(detector))
xml = await detector.hdf.nd_attributes_file.get_value()
for element in xml:
assert str(element.tag) == "Attribute"
assert (
str(element.attrib)
== f"{{'name': '{detector.name}-sum', 'type': 'PARAM', '"
+ "source': 'NDPluginStatsTotal', 'addr': '0', 'datatype': 'DBR_LONG',"
+ " 'description': 'Sum of the array'}"
)


async def test_nd_attributes_plan_stub(RE, detectors):
for detector in detectors:
await detector.connect(mock=True)
detector.set_name(type(detector).__name__)
param = adcore.NDAttributeParam(
name=f"{detector.name}-sum",
param="sum",
datatype=adcore.NDAttributeDataType.DOUBLE,
description=f"Sum of {detector.name} frame",
)
pv = adcore.NDAttributePv(
name="Temperature",
signal=epics_signal_r(str, "LINKAM:TEMP"),
description="The sample temperature",
)
RE(setup_ndattributes(detector.hdf, [pv, param]))
xml = await detector.hdf.nd_attributes_file.get_value()
assert str(xml[0].tag) == "Attribute"
assert (
str(xml[0].attrib)
== "{'name': 'Temperature', 'type': 'EPICS_PV', '"
+ "source': 'ca://LINKAM:TEMP', 'datatype': 'DBR_NATIVE',"
+ " 'description': 'The sample temperature'}"
)
assert str(xml[1].tag) == "Attribute"
assert (
str(xml[1].attrib)
== f"{{'name': '{detector.name}-sum', 'type': 'PARAM', '"
+ "source': 'sum', 'addr': '0', 'datatype': 'DBR_DOUBLE',"
+ f" 'description': 'Sum of {detector.name} frame'}}"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have included tests here that are equivalent to the ones I have removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardise handling of Read, Config and Hinted signals for StandardDetector
2 participants