Merge pull request #172 from ISISComputingGroup/reword_adrs

Reword adrs

Author: rerpha

doc/architectural_decisions/001-repo-structure.md

@@ -1,8 +1,8 @@
-# Repository structure
+# 1. Repository structure
 
 ## Status
 
-Current
+Current, partially superseded by [ADR 6](006-where-to-put-code.md)
 
 ## Context
 
@@ -15,25 +15,26 @@ Tom & Kathryn
 
 ## Decision
 
-We will create a `core` repository, and publish it on PyPI.
+We will create a `core` repository, called `ibex_bluesky_core`, and publish it on PyPI.
 
 This repository will provide core building blocks, including plan stubs,
 devices, and utilities which are generic and expected to be useful across
 different science groups.
 
-Beamline or technique specific repositories will then depend on the `core`
-repository via PyPI.
+~~Beamline or technique specific repositories will then depend on the `core` repository via PyPI.~~
+Superseded by [ADR 6](006-where-to-put-code.md).
 
-The core repository will not depend on `genie_python`, so that other groups
-at RAL can use this repository. The genie python *distribution* may in future
-depend on this repository.
+The core repository will not depend on [`genie_python`](https://github.com/isiscomputinggroup/genie), so that other
+groups at RAL can use this repository. The [uktena](https://github.com/isiscomputinggroup/uktena) python *distribution* 
+includes this repository as one of its included libraries.
 
-This `core` repository is analogous to a similar repo, `dodal`, being used at
-Diamond.
+This `ibex_bluesky_core` repository is analogous to a similar repo, 
+[dodal](https://github.com/diamondlightsource/dodal), being used at Diamond Light Source, or
+[apstools](https://github.com/BCDA-APS/apstools), being used at the Advanced Photon Source.
 
 ## Consequences
 
-- We will have some bluesky code across multiple repositories.
+- We will have some bluesky code across multiple locations.
 - Other groups should be able to:
   - Pull this code easily from PyPI
   - Contribute to the code without depending on all of IBEX's infrastructure

doc/architectural_decisions/002-use-ophyd-async.md

@@ -1,4 +1,4 @@
-# Use `ophyd-async`
+# 2. Use `ophyd-async`
 
 ## Status
 
@@ -9,13 +9,18 @@ Current
 We need to decide whether to use `ophyd` or `ophyd-async` as our bluesky
 device abstraction layer.
 
+| | Docs                                     | Source | PyPi |
+| - |-| - | - |
+| ophyd | [Docs](https://blueskyproject.io/ophyd/) | [Source](https://github.com/bluesky/ophyd) | [PyPi](https://pypi.org/project/ophyd/) |
+| ophyd-async | [Docs](https://blueskyproject.io/ophyd-async/) | [Source](https://github.com/bluesky/ophyd-async) | [PyPi](https://pypi.org/project/ophyd-async/) |
+
 `ophyd` has been the default device abstraction layer for bluesky EPICS devices for
 some time.
 
 `ophyd-async` is a newer (and therefore less mature) device abstraction layer, 
 with similar concepts to `ophyd`. It has been implemented primarily by DLS.
 
-The *primary* differences are:
+The *primary* differences which are relevant for ISIS are:
 - In `ophyd-async` many functions are implemented as `asyncio` coroutines.
 - `ophyd-async` has better support for non-channel-access backends (notably, PVA)
 - Reduction in boilerplate

doc/architectural_decisions/003-run-in-process.md

@@ -1,4 +1,4 @@
-# Run in-process
+# 3. Run in-process
 
 ## Status
 

doc/architectural_decisions/004-use-scipp.md

@@ -1,23 +1,70 @@
-# Use `scipp`
+# 4. Use `scipp`
 
 ## Status
 
 Current
 
 ## Context
 
-A decision needs to be made about whether to use scipp, numpy, uncertainties or develop our own library for the purpose of providing support for generating uncertanties on our counts data.
+We need to choose a library which helps us to transform "raw" neutron or muon data from the DAE, into processed
+quantities that we scan over.
+
+Desirable features include:
+- Uncertainty propagation, following standard uncertainty propagation rules. While this could apply to any data in
+principle, it will be especially relevant for neutron/muon counts data.
+- Unit handling & conversions
+  - Simple unit conversions, like microns to millimetres.
+  - Neutron-specific unit conversions, like time-of-flight to wavelength
+- Ability to handle the typical types of data we would acquire from the DAE and process as part of a scan:
+  - Histograms of neutron/muon counts
+  - N-dimensional arrays
+  - Event-mode data (in future)
+
+Candidate solutions include:
+
+- `mantid`
+- `scipp`
+- `uncertainties`
+- `numpy` + home-grown uncertainty-propagation
 
 ## Decision
 
-We will be using scipp.
+- Default to using `scipp` for most cases
+- Explore using `mantid` via autoreduction APIs, where we need to do more complex reductions
 
 ## Justification & Consequences
 
-- `scipp` is being developed at ESS with past input from STFC, so is well suited for neutron counts data.
-- `scipp` has a `numpy`-like interface but handles units and uncertainties by default under-the-hood.
-- Neither `numpy` or `uncertanties` have exactly the functionality we would need, so the solution using them would be a mix of the libraries and our own code, there would be more places to go wrong. Maintainability.
-- `uncertainties` package tracks correlations so may have bad scaling on "large" arrays like counts data from the DAE.
-- Developing our own uncertainties library will take time to understand and then implement. All of the functionality that we need has been done beforehand, so better to not waste time & effort.
-- Less expertise with this library on site (mitigation: don't do too much which is very complicated with it)
-- Potentially duplicates some of `mantid`'s functionality: (mitigation: use `scipp` for "simple" things, use `mantid` in future if people want to do "full" data reduction pipelines)
+### `numpy`
+
+Using `numpy` by itself is eliminated on the basis that we would need to write our own uncertainty-propagation code,
+which is error prone.
+
+`numpy` by itself may still be used in places where uncertainty propagation is not needed.
+
+### `uncertainties`
+
+The `uncertainties` package tracks correlations so may have bad scaling on "large" arrays, where correlation matrices
+can become large in some cases. Would need to be combined with another library, e.g. `pint`, in order to also support
+physical units. No neutron-specific functionality.
+
+### `mantid`
+
+Mantid is not easily installable (e.g. via `pip` at present).
+
+While we have a way to call out to mantid via a REST API, initial tests have shown that the latency of this approach
+is around 15 seconds. This means it is unsuitable for many types of scans, for example alignment scans, where count
+times are far lower than 15 seconds.
+
+However, for complex reductions, we should still consider the option of passing data out to mantid. This is especially
+true if reductions depend significantly on instrument geometry, on instrument-specific corrections, or on other details
+for which mantid is best-equipped to deal with.
+
+Calling out to mantid via an API should also be considered if a reduction step may use significant compute resource.
+
+### `scipp`
+
+`scipp` will be our default way of taking raw data from the DAE and processing it into a scanned-over quantity.
+
+However, in cases where the reduction is expensive (in terms of compute cost) or complex (either implementation, or
+in terms of required knowledge of geometry/instrument-specific corrections), then we should consider using mantid via
+the autoreduction API in those cases instead.

doc/architectural_decisions/005-variance-addition.md

@@ -1,24 +1,42 @@
-# Variance addition to counts data
+# 5. Variance addition to counts data
 
 ## Status
 
 Current
 
 ## Context
 
-For counts data, the uncertainty on counts is typically defined by poisson counting statistics, i.e. the standard deviation on `N` counts is `sqrt(N)`.
+For counts data, the uncertainty on counts is typically defined by poisson counting statistics, i.e. the standard
+deviation on `N` counts is `sqrt(N)`.
 
-This can be problematic in cases where zero counts have been collected, as the standard deviation will then be zero, which will subsequently lead to "infinite" point weightings in downstream fitting routines for example.
+This can be problematic in cases where zero counts have been collected, as the standard deviation will then be zero,
+which will subsequently lead to "infinite" point weightings in downstream fitting routines for example.
 
 A number of possible approaches were considered:
 
-| Option | Description |
-| --- | --- |
-| A | Reject data with zero counts, i.e. explicitly throw an exception if any data with zero counts is seen as part of a scan. |
-| B | Use a standard deviation of `NaN` for points with zero counts. |
-| C | Define the standard deviation of `N` counts as `1` if counts are zero, otherwise `sqrt(N)`. This is one of the approaches available in mantid for example. |
-| D | Define the standard deviation of `N` counts as `sqrt(N+0.5)` unconditionally - on the basis that "half a count" is smaller than the smallest possible actual measurement which can be taken. |
-| E | No special handling, calculate std. dev. as `sqrt(N)`. |
+### Option A
+
+Reject data with zero counts, i.e. explicitly throw an exception if any data with zero counts is seen as part of a scan.
+
+### Option B
+
+Use a standard deviation of `NaN` for points with zero counts.
+
+### Option C
+
+Define the standard deviation of `N` counts as `1` if counts are zero, otherwise `sqrt(N)`. This is one of the
+approaches [available in mantid](https://github.com/mantidproject/mantid/blob/bbbb86edc2c3fa554499770463aa25c2b46984e5/docs/source/algorithms/SetUncertainties-v1.rst#L16) for example.
+
+### Option D
+
+Define the standard deviation of `N` counts as `sqrt(N+0.5)` unconditionally - on the basis that "half a count" is 
+smaller than the smallest possible actual measurement which can be taken.
+
+### Option E
+
+No special handling, calculate std. dev. as `sqrt(N)`.
+
+---
 
 For clarity, the following table shows the value and associated uncertainty for each option:
 
@@ -53,6 +71,8 @@ The consensus was to go with Option D.
 ## Justification
 
 - Option A will cause real-life scans to crash in low counts regions.
-- Option B involves `NaN`s, which have many surprising floating-point characteristics and are highly likely to be a source of future bugs.
-- Option D was preferred to option C by scientists present.
-- Option E causes surprising results and/or crashes downstream, for example fitting may consider points with zero uncertainty to have "infinite" weight, therefore effectively disregarding all other data.
+- Option B involves `NaN`s, which have many surprising floating-point characteristics and are highly likely to be a
+source of future bugs.
+- Option D was preferred to option C by scientists present, because it is continuous.
+- Option E causes surprising results and/or crashes downstream, for example fitting may consider points with zero
+uncertainty to have "infinite" weight, therefore effectively disregarding all other data.

doc/architectural_decisions/006-where-to-put-code.md

@@ -1,4 +1,4 @@
-# Where to put code
+# 6. Where to put code
 
 ## Status
 

doc/conf.py

@@ -29,7 +29,8 @@
     ("py:obj", r"^.*\.T.*_co$"),
 ]
 
-myst_enable_extensions = ["dollarmath"]
+myst_enable_extensions = ["dollarmath", "strikethrough"]
+suppress_warnings = ["myst.strikethrough"]
 
 extensions = [
     "myst_parser",