Split out common AD file plugin logic into core writer class, create ADTiffWriter #606
Conversation
|
@genematx Here are the ophyd async changes that introduce tiff writer support, specifically you can look at |
There was a problem hiding this comment.
We reviewed the changes interactively with @jwlodek and @RobertSchaffer1, and they make sense to me.
@coretl, if you are happy with the changes, Jakub will rebase the PR branch to resolve the conflicts.
| @property | ||
| def include_file_number(self): | ||
| """Boolean property to toggle AD file number suffix""" | ||
| return self._include_file_number |
There was a problem hiding this comment.
Basically for toggling whether or not we want to include the _%6.6d suffix to the file names. Typically, with TIFF/JPEG, you would always, but with HDF5 it is more case by case - if we want to do a raster scan for example, with one row per HDF file, it would be nice if they could include the number - currently the HDF writer does not
There was a problem hiding this comment.
but we do we need it as a read-only property? Isn't it always used internal to that class?
There was a problem hiding this comment.
I made it a property to be able to switch the behavior at run time (depending on the experiment type for example). But I think just allowing that to be specified once at init is reasonable as well.
| ".h5", | ||
| "application/x-hdf5", | ||
| ) | ||
| self.hdf = cast(NDFileHDFIO, self.fileio) |
There was a problem hiding this comment.
We can avoid these casts by making the baseclass generic on it:
NDFileIOT = TypeVar("NDFileIOT", bound=NDFileIO)
class ADWriter(DetectorWriter, Generic[NDFileIOT]):
def __init__(
self,
fileio: NDFileIOT,
...
) -> None:
self.fileio = fileio # self.fileio is the same subclass of NDFileIO we passed in
...
class ADHDFWriter(ADWriter[NDFileHDFIO]):
def __init__(
self,
fileio: NDFileHDFIO,
...
) -> None:
super().__init__(
fileio,
...
)
async def open(self, multiplier: int = 1) -> dict[str, DataKey]:
await asyncio.gather(self.fileio.chunk_size_auto.set(True)) # pyright knows self.fileio is NDFileHDFIO
...| prefix, | ||
| path_provider, | ||
| adcore.ADHDFWriter, | ||
| hdf_suffix, | ||
| AravisController, | ||
| AravisDriverIO, | ||
| drv_suffix=drv_suffix, | ||
| name=name, | ||
| config_sigs=config_sigs, | ||
| gpio_number=gpio_number, | ||
| ) | ||
| self.hdf = self._fileio |
There was a problem hiding this comment.
I'm not a big fan of inserting another class into the heirarchy with many more arguments, could we move out the creation of the driver and writer classes into a utility function instead? I had a go with this idea and came up with:
class AravisDetectorTIFF(StandardDetector[AravisController]):
def __init__(
self,
prefix: str,
path_provider: PathProvider,
drv_suffix="cam1:",
tiff_suffix="TIFF1:",
gpio_number: AravisController.GPIO_NUMBER = 1,
config_sigs: Sequence[SignalR] = (),
name="",
):
self.drv, self.tiff, writer = adcore.areadetector_driver_and_tiff(
drv_cls=AravisDriverIO,
prefix=prefix,
drv_suffix=drv_suffix,
fileio_suffix=tiff_suffix,
path_provider=path_provider,
)
super().__init__(
controller=AravisController(self.drv, gpio_number),
writer=writer,
config_sigs=config_sigs,
name=name,
)I did some of the changes required so pyright was happy, but didn't make the tests work:
efd86c9
There was a problem hiding this comment.
Would it be possible to have the utility function(s) be factories that take in a drv_cls and writer_cls? It would be nice if we didn't need to make a function for each writer type. I am OK with that approach generally though
There was a problem hiding this comment.
That sounds reasonable, I wonder if we should stretch it even further and say that we make one AravisDetector that has an enum init arg fileio_type that switches between HDF and TIFF?
There was a problem hiding this comment.
That would be ideal I think. I'll work on getting something like that working.
There was a problem hiding this comment.
Hmmm, not sure I love that, but it would be nice to be able to specify the writer and have a default suffix. What if fileio_suffix=None, and
if fileio_suffix is None:
fileio_suffix = ad_writer_type.value
And have the enum values for the writer types just correspond to the default suffixes?
There was a problem hiding this comment.
Otherwise we use whatever the suffix kwarg provides?
There was a problem hiding this comment.
Found another possible approach which avoids the need to make detector specific subclasses:
ADBaseIOT = TypeVar("ADBaseIOT", bound=ADBaseIO)
NDFileIOT = TypeVar("NDFileIOT", bound=NDFileIO)
NDPluginBaseIOT = TypeVar("NDPluginBaseIOT", bound=NDPluginBaseIO)
class AreaDetector(
Generic[ADBaseIOT, DetectorControllerT, NDFileIOT],
StandardDetector[DetectorControllerT],
):
def __init__(
self,
drv: ADBaseIOT,
controller: DetectorControllerT,
fileio: NDFileIOT,
writer: DetectorWriter,
plugins: dict[str, NDPluginBaseIO],
config_sigs: Sequence[SignalR] = (),
name: str = "",
):
self.drv = drv
self.fileio = fileio
for name, plugin in plugins.items():
setattr(self, name, plugin)
super().__init__(controller, writer, config_sigs, name)
def get_plugin(
self, name: str, plugin_type: type[NDPluginBaseIOT] = NDPluginBaseIO
) -> NDPluginBaseIOT:
plugin = getattr(self, name, None)
if not isinstance(plugin, plugin_type):
raise TypeError(
f"Expected {self.name}.{name} to be a {plugin_type}, got {plugin}"
)
return plugin
@classmethod
def with_hdf(
cls,
drv: ADBaseIOT,
controller: DetectorControllerT,
hdf_prefix: str,
path_provider: PathProvider,
plugins: dict[str, NDPluginBaseIO],
config_sigs: Sequence[SignalR] = (),
name: str = "",
) -> AreaDetector[ADBaseIOT, DetectorControllerT, NDFileHDFIO]:
fileio = NDFileHDFIO(hdf_prefix)
writer = ADHDFWriter(
fileio,
path_provider,
lambda: det.name,
ADBaseDatasetDescriber(drv),
*plugins.values(),
)
det = AreaDetector(drv, controller, fileio, writer, plugins, config_sigs, name)
return det
@classmethod
def with_tiff(
cls,
drv: ADBaseIOT,
controller: DetectorControllerT,
tiff_prefix: str,
path_provider: PathProvider,
plugins: dict[str, NDPluginBaseIO],
config_sigs: Sequence[SignalR] = (),
name: str = "",
) -> AreaDetector[ADBaseIOT, DetectorControllerT, NDFileIO]:
fileio = NDFileIO(tiff_prefix)
writer = ADTIFFWriter(
fileio, path_provider, lambda: det.name, ADBaseDatasetDescriber(drv)
)
det = AreaDetector(drv, controller, fileio, writer, plugins, config_sigs, name)
return detSo now det.drv and det.fileio are always present, and we can do det.get_plugin("stats", NDPluginStatsIO) if we created it with AreaDetector(..., plugins={"stats": NDPluginStatsIO(prefix+"STATS")}
There was a problem hiding this comment.
So the detector specific classes turn into helper functions:
def _aravis_drv_controller(
prefix: str, drv_suffix: str, gpio_number: AravisController.GPIO_NUMBER
) -> tuple[AravisDriverIO, AravisController]:
drv = AravisDriverIO(prefix + drv_suffix)
controller = AravisController(drv, gpio_number)
return drv, controller
def aravis_detector_hdf(
prefix: str,
path_provider: PathProvider,
drv_suffix="cam1:",
hdf_suffix="HDF1:",
gpio_number: AravisController.GPIO_NUMBER = 1,
plugins: dict[str, NDPluginBaseIO] | None = None,
config_sigs: Sequence[SignalR] = (),
name="",
) -> adcore.AreaDetector:
drv, controller = _aravis_drv_controller(prefix, drv_suffix, gpio_number)
hdf_prefix = prefix + hdf_suffix
return adcore.AreaDetector.with_hdf(
drv, controller, hdf_prefix, path_provider, plugins or {}, config_sigs, name
)
def aravis_detector_tiff(
prefix: str,
path_provider: PathProvider,
drv_suffix="cam1:",
tiff_suffix="TIFF:",
gpio_number: AravisController.GPIO_NUMBER = 1,
plugins: dict[str, NDPluginBaseIO] | None = None,
config_sigs: Sequence[SignalR] = (),
name="",
) -> adcore.AreaDetector:
drv, controller = _aravis_drv_controller(prefix, drv_suffix, gpio_number)
tiff_prefix = prefix + tiff_suffix
return adcore.AreaDetector.with_hdf(
drv, controller, tiff_prefix, path_provider, plugins or {}, config_sigs, name
)
|
Applied the changes, rebased, and tests are now passing, but for some reason I am having issues running with real hardware. I think I will start a fresh environment on Monday and confirm if I can collect both HDF and TIFF data and read it back via tiled. |
|
I can see why you've done it, but I would still prefer to pass in the writer instance rather than class into the |
|
This is now working and tested with real devices: |
|
One thing to note - the hanging I mentioned in real hardware tests was actually not caused by any change I made in this PR, but rather because of this line: needing the same kwarg that the driver start acquiring command takes. Otherwise it will hang there forever.Probably should be fixed in main if we will hold off on this PR for a bit longer. |
| writer, fileio = writer_cls.writer_and_io( | ||
| prefix, | ||
| path_provider, | ||
| lambda: name, |
There was a problem hiding this comment.
| lambda: name, | |
| lambda: self.name, |
Or event better let writer_cls.writer_and_io do this as in my PR
| @@ -87,7 +93,7 @@ def get_deadtime(self, exposure: float | None) -> float: | |||
| """For a given exposure, how long should the time between exposures be""" | |||
|
|
|||
| @abstractmethod | |||
| async def prepare(self, trigger_info: TriggerInfo): | |||
| async def prepare(self, trigger_info: TriggerInfo) -> Any: | |||
There was a problem hiding this comment.
Is the return value of this always going to be the same for a given DetectorController, or is it going to differ depending on the type of experiment? DetectorController could be generic on the return value of prepare, so we have some type checking?
There was a problem hiding this comment.
I don't think this should ever return anything...
@jwlodek are there examples where it does return something?
There was a problem hiding this comment.
No, I don't think so. For some reason vscode built in type checking was reporting that it was returning a coro and such having a type mismatch. I think it should just be None though. I will switch it back
|
Confirmed I can read images back w/ this & bluesky main, most recent beta release of tiled: |
This now seems to mostly work, at least from the collection side.
The data cannot be read back via tiled at the moment, however, so for now marking as draft, since I may need to edit the docs generated somewhat to account for this. PRs intoblueskyandtiledhave been merged to allow reading back collected TIFF data with this branch.Addresses #595 .
TODO: