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

Make it easier to use area detectors with a factory #987

Merged
merged 14 commits into from
Aug 27, 2024
Merged

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian self-assigned this Jun 28, 2024
@prjemian prjemian added this to the 1.6.21 milestone Jun 28, 2024
@prjemian prjemian marked this pull request as draft June 28, 2024 21:39
@prjemian
Copy link
Contributor Author

The initial commit is for a fixed version of ophyd area detector support. It is templated on the support provided for the APS XPCS instrument.

@prjemian
Copy link
Contributor Author

Still need to add/blend contribution by @cooleyv that requests the ADCore version from the existing PV and use case from @canismarko

The challenge with asking the ADCore version is that the IOC must be running enough to respond.

@prjemian
Copy link
Contributor Author

Will be a fun challenge blending these and then building unit tests.

@prjemian
Copy link
Contributor Author

prjemian commented Aug 23, 2024

The factory method looks too complex. Can it be simpler? (advice and here)

@prjemian
Copy link
Contributor Author

There are very few detectors in use at APS that still use ADCore releases older than 3.4. Additional support for versions of the various plugins is not justified. They can be addressed individually if the new factory design is not adequate.

@prjemian
Copy link
Contributor Author

prjemian commented Aug 23, 2024

Needs some tests for ophyd.sim.instantiate_fake_device(), as suggested by @canismarko.

@prjemian
Copy link
Contributor Author

prjemian commented Aug 24, 2024

Instead of:

REMOVE_DEFAULT_KEY = object()
"""Flag to remove a plugin configuration key."""

to remove a default, such as:
det = ad_creator(
"MyEiger", "ad:", "det",
[
{"cam": {"class": EigerDetectorCam}},
"image",
{"hdf1": {"write_path_template": REMOVE_DEFAULT_KEY}},
],
)

allow the caller to re-define a key with defaults kwarg which will replace the PLUGIN_DEFAULTS dictionary:

PLUGIN_DEFAULTS = { # some of the common plugins

@prjemian prjemian marked this pull request as ready for review August 24, 2024 21:56
@prjemian
Copy link
Contributor Author

@cooleyv, @keenanlang, @canismarko, @gfabbris, @strempfer, @MarkRivers -- The new area detector factory support is ready for review.

@prjemian
Copy link
Contributor Author

@sureshnaps : Your suggestion has led to simplifications such as.

    from ophyd.areadetector import SimDetectorCam
    from apstools.devices import ad_creator

    det = ad_creator(
        "ad:", name="det", class_name="MySimDetector",
        [
            {"cam": {"class": SimDetectorCam}},
            "image",  # all the defaults are acceptable
            "pva",
            {"hdf1": {"write_path_template": "/", "read_path_template": "/mnt/ioc/tmp"}},
            "codec1",
            "proc1",
            "roi1",
            "stat1",
        ],
    )

@prjemian
Copy link
Contributor Author

In the PLUGIN_DEFAULTS, the origin (ophyd v. apstools) of the default class for each plugin should be more obvious. Use absolute paths for the classes from ophyd.

Copy link
Collaborator

@MDecarabas MDecarabas left a comment

Choose a reason for hiding this comment

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

Looks good, can you make an issue to remove the local version out of 8-ID and transition to the APS tools version. We can make the changes at the same time we re-implement specwriter

Copy link
Collaborator

@canismarko canismarko left a comment

Choose a reason for hiding this comment

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

This strikes me as a positive change.

The difference between the plugins and plugin_defaults arguments to the AD factory is tricky for me to understand. I think this is because of the complicated way area detectors are built in ophyd rather than a design issue for the factory, so probably this needs to be well documented.

apstools/devices/tests/test_issue984_ad_factory.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@canismarko canismarko left a comment

Choose a reason for hiding this comment

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

Updating my reivew based on Pete's point that these actually need to be keyword arguments in the examples.

apstools/devices/area_detector_factory.py Show resolved Hide resolved
apstools/devices/area_detector_factory.py Outdated Show resolved Hide resolved
apstools/devices/area_detector_factory.py Outdated Show resolved Hide resolved
apstools/devices/area_detector_factory.py Show resolved Hide resolved
ophyd.areadetector.DetectorBase,
)
plugins = plugins or ["cam"]
plugin_defaults = plugin_defaults or PLUGIN_DEFAULTS
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes it impossible to pass an empty dict ({}) to this function as plugin_defaults, since bool({}) evaluates to False and then PLUGIN_DEFAULTS would get used.

Maybe not a big deal since I can't imagine many people would want this. But I think it could be avoided by using PLUGIN_DEFAULTS as the default value for the plugin_defaults, which makes the intention more clear when looking at the call signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same for plugins=None. Changing it to plugins=["cam"].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... there's the reason to keep the default kwarg value of None -- in the unit tests, None is used.

ERROR apstools/devices/tests/test_area_detector_support.py::test_AD_EpicsFileNameMixin[hdf1-AD_HDF5] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_AD_EpicsFileNameMixin[jpeg1-AD_JPEG] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_AD_EpicsFileNameMixin[tiff1-AD_TIFF] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_stage[hdf1-AD_EpicsFileNameHDF5Plugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_stage[jpeg1-AD_EpicsFileNameJPEGPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_stage[tiff1-AD_EpicsFileNameTIFFPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_bp_count_custom_name[hdf1-AD_EpicsFileNameHDF5Plugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_bp_count_custom_name[jpeg1-AD_EpicsFileNameJPEGPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_bp_count_custom_name[tiff1-AD_EpicsFileNameTIFFPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_full_file_name_local - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_no_file_path - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_file_numbering - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_capture_error_with_single_mode - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_single_mode - TypeError: type.__new__() argument 2 must be tuple, not None

Code like this is needed:

    if plugin_defaults is None:
        plugin_defaults = PLUGIN_DEFAULTS

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, sounds good.

Copy link
Contributor Author

@prjemian prjemian Aug 26, 2024

Choose a reason for hiding this comment

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

I can't imagine many people would want plugin_defaults={}

Actually, this is a realistic case to remove all defaults and force the caller to define all keywords of each plugin. Probably only useful for unit testing, not real world?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@canismarko Will these changes work for you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, changes look good to me. Thanks!

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 also added some documentation (look for EXAMPLE 1: and others).

@prjemian prjemian merged commit f0658de into main Aug 27, 2024
10 of 13 checks passed
@prjemian prjemian deleted the 984-AD-factory branch August 27, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

area detector factory function
3 participants