Rework LPD Mini components class

[takluyver]

This was in need of a rework because we have discovered since adding it that LPD Mini is actually several smaller detectors recorded as a single source in the DAQ, rather than one 256 x 256 detector.

This exposes each module (with 2 tiles) as a 32 x 256 strip, as it is saved, rather than the 64 x 128 arrangement they make physically. The idea is that the geometry will be responsible for putting the data together as an image: European-XFEL/EXtra-geom#187

This is a WIP because I'm still figuring some parts out, and of course it needs tests and docs, but there's something there one can try - see https://max-jhub.desy.de/user-redirect/notebooks/Code/EXtra-data/LPD%20Mini%20test.ipynb

[philsmt]

Is this MR still WIP or ready for review?

[takluyver]

Right, sorry, I think this is ready for review. We've settled on the source name for corrected data (FXE_DET_LPD_MINI/CORR/0CH0:output) and the shape (frames, modules, 32, 256).

This is also less critical, though - we're not using this in the calibration code, and as the detector is a single source, it's relatively easy to access the data directly with normal EXtra-data functionality. The main place where this will be useful is to efficiently read e.g. the first pulse from each train.

[philsmt]

We briefly discussed the conundrum between potentially offering non-native/"default" axis orders like module, frame, *xy in this component and the interaction with the out= argument to .ndarray(). Since automatic re-ordering to conform to other detectors seems like a typical application for high-level components, we decided to offer it and just disable the out= argument if any non-native axis ordering is chosen.

[takluyver]

Thinking through that again, I'm not sure. Would we make reordering axes the default?

If we don't, then to match other detectors you have to know that it's different and explicitly do something extra, in which case you could just as well call np.moveaxis() or np.transpose().

If we do reorder by default, then to use an out= parameter you have to prepare the output buffer in a different shape from normal output and pass an extra parameter to specify the one axis order that will work. Also, (frames, modules, ...) is possibly a more 'obvious' order than (modules, frames, ...), even if it's not what other detectors do, because you often want to look at all modules for the same pulse.

[philsmt]

I gravitate towards the option most helpful to novice programmers that may have a harder time doing the np.transpose themelves, and/or are less likely to realize there's a keyword argument to change its behaviour if they want to use out= (assuming the exception makes this clear).

I was having a similar philosophical discussion the other day about TOF/REMI components I am working on, and it lead me to the conclusion the extra_data.components should make data accessible first, and less repetitive second, i.e. prioritize dealing with inexperience over convenience.

[takluyver]

I gravitate towards the option most helpful to novice programmers that may have a harder time doing the np.transpose themelves, and/or are less likely to realize there's a keyword argument to change its behaviour if they want to use out= (assuming the exception makes this clear).

I'm honestly confused now about which way you're pushing for. At first I thought the most helpful thing for novice programmers was to do the reordering (to modules, frames, *px) by default, but that's the option that requires an extra keyword argument to use out=.

In isolation, I think the natural order from LPD Mini (frames, modules, *px) is actually better than doing the reordering - my experience suggests you select & reduce on the frames dimension more often. And having to change the axis order to use out= is certainly worse. The only reason in favour of the reordering is for (partial) consistency with other detectors.

So I lean towards leaving this as is, and maybe as a separate PR introducing an axis_order= parameter across all detectors to make rearranging axes convenient.

[philsmt]

I suspect the meaning got lost in translation, so let me try again: the group of people stumbling over a different shape and how to fix it does not intersect with those using out=.

I don't disagree about the natural order, but would keenly like to avoid different detectors returning different shapes.. There's already a lot of messiness out there with regard to shapes or other such details that I would like to not add to it

[takluyver]

I started on implementing this, but... the inconsistencies it leaves within the API are uglier than I thought. E.g. should lpdmini['image.data'].buffer_shape() match the shape of what .ndarray() returns, or the shape you need for a buffer to pass to .ndarray(out=)? Either way, there's an inconsistency.

I also realised that there's another difference from other detectors in things like pulseId & cellId - these will be 1D arrays, whereas for the other detectors they're 2D. And 1D is better! They're meant to be consistent across modules. So do we duplicate them along the module dimension, being actively unhelpful, just to match other detectors?

I appreciate the argument that the API should prioritise novice users. But making the API inconsistent with itself in order to make it partially consistent with other detectors, and convoluting the code to make it so, feels like a bad trade-off. I think we're better off saying that LPD Mini works a bit differently from other detectors, and if you need to rearrange the axes, here's how to do it (incidentally showing people a numpy function that may be useful elsewhere).