[tomkane-dls]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 62.96296% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.74%. Comparing base (eace3a3) to head (5d7e047).

Files with missing lines	Patch %	Lines
src/test_rig_bluesky/plans.py	28.57%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
+ Coverage   50.70%   52.74%   +2.03%     
==========================================
  Files           5        5              
  Lines         213      237      +24     
==========================================
+ Hits          108      125      +17     
- Misses        105      112       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hyperrealist]

Thanks Tom, nice to have the diodes coming in. Since it's still a draft, here are some things that would help the data land in Tiled in a shape ViSR can use. None of it is blocking; details are in the inline comments.

Dataset names on the pandabox (the main one). The pandabox's HDF datasets become data keys in primary exactly as named by its *:DATASET PVs (ophyd-async adds no device prefix), but nothing in the plan sets them: there is no pandabox design loaded the way pandabrick_baseline is for the pandabrick. So what reaches Tiled depends on how the IOC happens to be set up, and with an empty DATASETS table there would be no diode data at all, just a warning. A pandabox_baseline.yaml loaded with load_panda_settings would pin which FMC_IN channels are captured and what they're called. Two things to keep in mind when choosing:

• The names share primary with the camera's and the pandabrick's (X, Y), so they must be unique, and ideally match the step-scan keys (spectroscopy_diodes-red etc.), so a reader finds the diodes the same way in step and fly scans.
• With capture Min Max Mean, the file gets <name>, <name>-min and <name>-max, but only <name> (the mean) is listed in DATASETS, so only the mean is registered in Tiled. If min/max matter, they'd need their own dataset entries.

Related, from reading fastcs-pandablocks (which the pandabox runs, 0.2.2): DatasetTableWrapper.get_numpy_table compares the capture enum with the string "No", which never matches, so a field that still has a name but capture No stays in DATASETS and gets registered, though the file never has it. I haven't confirmed it on the IOC, but naming every dataset explicitly in the design avoids it either way.

Step and fly values mean different things. In a step scan the diodes are read from the live FmcIn:Val PVs at read time, not over the camera's exposure; in a fly scan they're the PCAP mean per frame. Worth deciding which one is wanted for step scans (could they come from the pandabox's PCAP too?), or at least noting it in the metadata.

Gain. It's a configuration signal, so it's recorded once per scan. With "Auto" the range can change mid-scan, and the raw counts aren't comparable across points unless the range is recorded with each point (e.g. gain_readback as a reading rather than config, which would be in dodal#2235), or "Auto" is ruled out for scans.

Tests. test_spectroscopy, test_spectroscopy_defaults, test_spectroscopy_datasets, test_spectroscopy_sets_exposure_time_and_acquire_period and test_demo_spectroscopy are deleted rather than updated; it'd be good to have them back with mock pandabox and diodes, test_spectroscopy_datasets especially, since it's what pins the data keys.

Dependency. RGBDiodes comes from dodal#2235, so dls-dodal>=1.56.0 will need raising to the release that has it.

On the Tiled side, ViSR's /binned currently reads only RedTotal/GreenTotal/BlueTotal; we're making the channel names configurable, so once the diode names are settled, it can show them.

[hyperrealist]

This records one reading of the live values (and the gain, as configuration) at the start, in a diodes stream. Fine as a snapshot; just noting that the per-point diode data in fly scans comes only from the pandabox datasets, so their names are what a reader needs.

[shihab-dls]

Looks good! Just a few comments. Seems we need to add to the tests to cover the new panda and diodes checks, so I'll comment for now and come back to this when CI is green

[EmsArnold]

It seems slightly odd that we're loading settings for the pandabox here, while we load settings for the pandabrick inside the fly_scan plan. Is there a reason to keep it outside?

[tomkane-dls]

I suppose because the pandabrick is used by both experiments but the pandabox is only used by spectroscopy.

but looking at things now, the split of what is in spectroscopy() and what is in fly_scan() is a little arbitrary. The idea was that fly_scan was the same core function needed by both spectroscopy and tomography, but now we are passing diodes and pandabox its lost a bit of this.

[tomkane-dls]

Do you suggest injecting pandabox inside plans.py?

[EmsArnold]

I'm not sure which way would be better. With the pandabox and diodes inside fly_scan, I expected to see any settings loading happening there (as it does with the pandabrick). Is there a reason that we couldn't have something link extra_detectors, and expect the outer plan to set up any extra detectors (diodes and pandas for the spectroscopy scan in our case) before we set the fly scan going?

[EmsArnold]

After another look, that won't work either as the pandabox needs kicking off and you want to read the diodes beforehand.
I'm not sure there's an elegant solution here, other than stick a load of conditions into the generic scan.

[tomkane-dls]

@EmsArnold I can't see an elegant way of doing this currently, a shared fly_scan seems to be getting more complicated and uglier, but i'm not exactly sure separating it out again for each plan is likely to make it better. I will investigate whether two completely separate plans might look better on a new PR once tomography is working.

[EmsArnold]

LGTM - thanks!