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

DetectorParams should be simplified #966

Open
rtuck99 opened this issue Dec 19, 2024 · 0 comments
Open

DetectorParams should be simplified #966

rtuck99 opened this issue Dec 19, 2024 · 0 comments

Comments

@rtuck99
Copy link
Contributor

rtuck99 commented Dec 19, 2024

Issues

There are a number of issues with DetectorParams:

  • Accessing some properties in DetectorParams have side effects (computation of values dependent on system state, loading of configuration files and accessing of directories) and this is not obvious to code that uses it.
  • Accessing the detector_params property of experiment params has side effects (construction of directories, filesystem access, both in the accessor and in the model validation)
  • This leads to excessive filesystem accesses
  • The purpose of DetectorParams seems to be somewhat confused. Partly it is used for setting up the detector. But it is also used to transmit information about the detector to callbacks, and for those callbacks to compute values in a device specific way without access to the device itself.
  • Some of the information in DetectorParams is duplicated elsewhere in the parameter model

beam_xy_converter - why is this in DetectorParams? It shouldn't be in the detector params, it should be associated with the dodal device. The only reason it is there is because we need to use it in the callbacks and callbacks don't have knowledge about device internals, only parameters and data.
What we should really be doing is calculating it when we fire the event and populating the callback events with the data.

omega_start and omega_increment - are information about the expt and not the detector. Probably should be reading this from a ScanSpec, rather than carrying around a duplicate set of info. Then different detectors with different experiments could all share the same interface regardless of what kind of trajectory they are following.

run_number - The run_number property reads the directory to determine the run number, but then doesn't apply the override, so it can change over time!
We shouldn't leave the value of this property up to the vagaries of when we decide to access it. We should explicitly compute and set it at a time of our choosing in the experiment plan. It shouldn't even be a parameter, the paths that derive from this should just be signals on the device that we set explicitly, and the mechanism to compute it should just be an ordinary free function.

Similar arguments I think also apply to directory, it should not be part of the parameters, it should be set in the plan and callbacks should read it back from the device readings.

Relation to the main experiment parameters.

With the above changes I think it should be possible to compute all the necessary info for the remaining properties in DetectorParams up front, make it immutable and then we can eagerly instantiate it, not have to do file accesses or compute values on property access, we can just make it a dumb pydantic dataclass that does type validation which is what it should be in the first place.

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

No branches or pull requests

1 participant