add: live gridded reciprocal-space volume (4/4) - #139
Conversation
69b76d2 to
6c8c969
Compare
188fdbb to
ec48bc7
Compare
6c8c969 to
5cf7012
Compare
3e547ea to
b2c60b2
Compare
|
The crash you hit here came from the same root cause as #137 — fixed in A bad profile no longer makes This branch was rebased (it had a duplicate cherry-pick of the fix), so please |
|
Requested by Osayi-ANL on #139: the HKL Setup editor and the HDF5 writing path both changed enough to warrant a minor bump ahead of merging the RSM stack.
6731035 to
9d77d11
Compare
5cf7012 to
d575f4c
Compare
|
Went through each item:
Current tip is |
There was a problem hiding this comment.
-
Change these '{"CENTER_CHANNEL_PIXEL": [300.0, 300.0], "DISTANCE": 400.644, "PIXEL_DIRECTION_1": "z-", "PIXEL_DIRECTION_2": "x-", "SIZE": [28.38, 28.38], "UNITS": "mm"}' to a single input in the GUI. You don't have to change the dict structure it was saved in just how we grap the values.
-
Sample Orientation input in the IOC_RSM_PARAMETER is not information that is used in 6IDB. We already have the Primary, Inplane, Sample directions which we don't necessarily use, but can be used in place of the input. So it is just redundant.
-
The HKL Setup does not populate based on the profile selected instead it selects the profile toggled in the setup config. Try opening the config and viewing an selected profile. Then open the HKL Setup. It opens that profile even though it is not selected.
…DETECTOR_SETUP into per-value fields Apply & Save and Reload both ran one check -- RAW_CONFIG != snapshot -- and reported every failure as "the IOC did not activate that snapshot", with a Retry button offered unconditionally. Three unrelated situations were being described with one wrong sentence, in vocabulary no beamline user can act on. Reproduced the report from #139: the editor is pinned to the profile it opened with (the IOC builds its record database from that profile at launch), but app_settings.reload() re-resolves the locator on every call, so selecting a different profile in another window makes the two diverge. The save had succeeded; nothing was corrupted; removing PVs was incidental. Retry could never have worked because the target had moved. The three cases are now distinguished and worded for someone who knows diffraction, not this codebase: - ActiveProfileChanged -- names both profiles ("HKL Setup is editing 'X', but 'Y' is now active"), states that the angle IOC is still serving X's PVs so live readers are unaffected, tells the user to reopen HKL Setup, and hides Retry because it cannot succeed. - ProfileContentMismatch -- the same profile came back with different values, i.e. a concurrent writer. Lists the differing settings in HKL terms ("Detector circle 1 - POSITION: saved ..., read back ...") under Show Details, capped at 15 with an overflow line. Retry still offered. - Anything restart_ioc raises -- the IOC genuinely failed to restart. Says the PVs still carry the previous values and nothing was rolled back. Profiles are named, never shown as database row ids. Detecting a switch needs the *resolved* profile, not app_settings.LOCATOR, which is None whenever the profile is auto-detected from the database selection -- comparing locators reports "unchanged" across exactly the switch this is meant to catch. Adds ConfigSource.resolved_identity() for that. Two adjacent fixes found on the way: - set_locator(None) wrote the literal string 'None' into the environment and the state file, so the next read returned it as a real, unresolvable locator. It now clears both, and _parse_locator treats a stale 'None' as unset so already-poisoned state files recover. - The out-of-sync text claimed "the profile is saved" even on the Reload path, which never saves. Also #139: DETECTOR_SETUP was a single line edit holding raw JSON. It is now one input per value, generated from DETECTOR_SETUP_FIELDS so the optional entries (PIXEL_SIZE, ROI, BINNING, DETROT/TILT, per-field units) each get a row with a placeholder showing the expected shape. Blank means absent. The stored table is untouched, including int-vs-float types, and unrecognised keys are carried through rather than dropped.
|
Thanks for the error text — that was the whole ballgame. Reproduced it exactly, and it's not what either of us assumed. The "IOC out of sync" errorIt was a profile switch, not a failed save, and nothing to do with removing PVs. The tell is in your own message: both the prefix and the record names differ — What happened: HKL Setup binds to whichever profile was active when it opened, because the IOC builds its EPICS record database from that profile at launch. But Your save went through fine. Nothing was corrupted and no rollback was needed. Do you remember having a second window open, or switching profiles in Workflow around then? That would confirm it end to end. What changed (
|
There was a problem hiding this comment.
Yes, I think an advanced section or tab to place the 'Sample Orientation' dropdown is necessary to not overstimulate the user. Let's do it that way.
- The protected_static_values change in 51cfa30 (rsm/02-naxis-issue132) reasserts profile values back onto the IOC on a timer, so caput can't be used from the terminal. We want all PVs assertable from the terminal.
I already approved #137, so I have a patch ready: it removes protected_static_values and the reassert loop. Static records still get set once at IOC startup. They track the ioc value and if the ioc is changed via caput or the gui a save dialog comes up and lists the changes. So caput changes can be detected by the ioc to be applied and saved to the profile.
Do you agree? I think that we should still allow caput's from the terminal to also be saved to profiles.
-
I don't think we need to save the AXIS_NUMBER as a dataset just a scalar is fine since it doesn'tchange
-
There are three ways we save a name associated to a PV(Label, Record Name, and Spec Motor Name). I don't think it needs to be that much maybe removing spec motor name.
|
@Osayi-ANL — status update after re-reviewing current stacked head Resolved on the current head
Remaining takeover blockers
Restack and gates
|
d575f4c to
3b9b10c
Compare
86e2605 to
d87cede
Compare
3b9b10c to
5b43d9c
Compare
d87cede to
d5aa57d
Compare
5b43d9c to
66fb8e7
Compare
setup GUI, and resolved-profile-identity handling (PR #139) The branch was restacked onto a new live-gridded-volume architecture and two reviewed pieces of prior work never made it back: an optional live-PV source for the UB matrix (backup/rsm-04-live-grid-pre-restack@2db947b) and a per-field detector-setup form plus a profile-identity fix for Apply & Save's out-of-sync messaging (@b500b8b). Ported as design reference only -- the surrounding change-review/CAS/persistence code has all changed since. IOC_RSM_PARAMETER gains two optional fields, additive to schema version 1: UB_MATRIX_SOURCE_PV and DETECTOR_SETUP.DISTANCE_SOURCE_PV. Each is validated as PV-name-only (a numeric string would pass the axis/energy SOURCE_PV convention's leniency but can never connect to anything, since UB_MATRIX and DETECTOR_SETUP.DISTANCE already have their own dedicated literal fields). UB_MATRIX/DISTANCE stay the validated static fallback either way. At IOC runtime, source_vector_or_fallback/source_scalar_or_fallback poll through the same cached EpicsPV(auto_monitor=True) mechanism as the existing per-axis source_value -- but fall back to the stored profile value instead of NaN on disconnect: a bad UB matrix or distance silently corrupts every downstream HKL index, unlike a stale axis angle. Logs only on failure/ recovery transitions. _adoptable_records excludes the UB/distance IOC records from live-caput adoption whenever a source PV is configured, keeping the existing unconditional adoption when it isn't. GUI: "UB Matrix (PV or value)" and "Distance (PV or value)" are dedicated widgets pulled out of the generic key.replace('_',' ').title() calibration loop (which produced "Ub Matrix"). Detector setup gets its own two-column grid generated from DETECTOR_SETUP_FIELDS, one row per declared field, placeholders showing the expected shape, direction-1/direction-2 wording throughout (never x/y). Both PV-or-value widgets track the last valid literal live via textChanged, not just on Apply -- otherwise typing a literal then switching to a PV in the same session would silently discard the just-typed value and fall back to whatever was last saved. DISTANCE_SOURCE_PV is excluded from the detector-setup "unknown key" carry-through, or switching back to a literal would leak the stale PV name back in on save. The close-time change-review table collapses each PV-or-value pair into one logical row instead of two independent flattened keys, so a keep/drop decision can't update only one half of the pair. Restores the three-way distinction Apply & Save lost: ActiveProfileChanged (the profile was switched in another window -- no Retry, it can't succeed), ProfileContentMismatch (same profile, different values -- a concurrent writer, Retry still offered), and a genuine restart failure. Detecting a switch needs ConfigSource.resolved_identity() (new), not app_settings.LOCATOR, which is None under DB auto-detect. The activation-mismatch diff also fixes a bug where an empty-dict section and an absent key flattened identically, letting a genuine mismatch report zero differences. set_locator(None) now clears the env var and state file instead of writing the literal string "None" into both; _get_effective_locator treats an already-poisoned "None" as unset so existing state files self-heal.
staleness warning (PR #139) Start Metadata Associator built its --metadata-channels string from whatever app_settings.HKL/METADATA_CA/METADATA_PVA already held in memory -- never reloading first, never checking the result actually covers what required_rsm_channels() needs to compute Q, and only deduping within the HKL-derived channels rather than across all sources. A profile missing a required channel would start a half-configured associator with no warning. run_associator_consumers now reloads app_settings first, then refuses to start and lists the missing channel names if required_rsm_channels(HKL) isn't fully covered by the built channel set. _build_metadata_channel_list returns the ordered URI list and the bare-name set together, rather than having the validation step re-split the joined ','-delimited string on ',' then '://' -- a free-text METADATA_CA/METADATA_PVA value containing a literal comma (a plausible Settings-tree typo) split into a fragment with no '://', raising an uncaught IndexError before the associator could ever start. _sync_associator_metadata no longer silently stops and restarts a running associator when its channel set or resolved profile identity drifts -- that could interrupt live acquisition mid-scan, and that decision belongs to the user. It now shows one non-modal "Metadata configuration changed -- restart required" notice instead. HKL Setup edits IOC_RSM_PARAMETER from a separate OS process and never calls back into Workflow, so a new ~10s QTimer polls for drift while an associator is running -- otherwise a config change made over there would never be noticed. The notice compares against the last *observed* state and always advances that baseline on detection, rather than gating on a run-start one-shot flag: a persisting drift still only notifies once (the next tick already matches the new baseline), but a later, different drift -- a second profile switch, or an earlier transient blip clearing -- is still caught. A one-shot-per-run flag would have permanently swallowed every notice after the first. Stopping the associator now also closes any lingering notice box instead of leaving it on screen telling the user to restart something they already stopped.
PV textbox text not vertically centered Start Live View / Stop Live View sat in their own QHBoxLayout in one grid column, while Provider/State/PV Input Channel lived inside "Connection", a QGroupBox in the other column whose three info rows were each an absolutely- positioned QWidget (Qt Designer's raw, unmanaged-layout pattern) rather than real layout items -- two unrelated containers that were never going to line up. The group box also had a self-contradictory size constraint (minimumSize.height=90, maximumSize.height=70), and pv_prefix was capped at a fixed 40px height in the .ui while theme.qss demands min-height: 50px; boxed inside a sibling container hard-fixed at 42px by absolute geometry, the line edit's real (taller) rendered height had nowhere to go -- the visible, clipped remainder is what read as text sitting at the bottom of the field. Moved the buttons into the same QHBoxLayout as the Connection group box (collapsing the grid's two-column row into one colspan=2 row), replaced the three absolutely-positioned sub-widgets with a real nested QHBoxLayout of QFormLayouts so the box actually resizes to fit its content, dropped the group box's conflicting maximumSize, and gave pv_prefix room to render at its real 50px height instead of overflowing a too-short fixed parent. No Python changes: hkl_3d_viewer.py only wires signals to these widgets by object name, none of which changed; verified no code references the removed container widgets. Confirmed visually (offscreen-rendered before/after) and via pytest -- 718 tests still pass, ruff clean.
…tic geometry PVs with frozen timestamps stop being discarded on every frame
…l QueueEmpty exception, ~8.4 ms/frame at ~47 channels remove: forced logger.setLevel(DEBUG) in __init__ — it flooded general.log every frame and throttled the consumer to ~10 Hz
…tor — it logged on every discarded channel and the nMetadataDiscarded counter already reports the same thing in the status channel
… IOC-only caput must make the close gate dirty through the same _pending_change() Apply uses, and a divergent form/IOC edit must stay a conflict that attempts no save at all change: the adoption implementation itself needed no change — both baselines, absent-key/raw-type/extension preservation and exact axis origins were already in place; only these two behaviours were unguarded
…_checkable_state, restored from f3efe69 which was lost when this branch was rebased change: the RSM parameter IOC editor's Advanced group is now a collapsible section, collapsed by default, with its expand state persisted under advanced_expanded
…e sections too, expanded by default, with their expand state persisted alongside Advanced add: CollapsibleSection now takes a layout class and an area objectName, so Detector setup keeps its two-column QGridLayout and the theme.qss rule that sizes its inputs change: retarget the detector-input qss selector from QGroupBox#groupBoxDetectorSetup to #groupBoxDetectorSetup — the container is a QFrame now, and the type-qualified selector would have silently stopped matching
…ator-speedup fix: metadata associator discarded all metadata and ran at ~10 Hz
…ated LABEL in every shipped config, and its only distinct behaviour was falling back to LABEL when blank change: axes now publish three IOC records each (AxisNumber, DirectionAxis, Position) instead of four, and SPEC_MOTOR_NAME is no longer a resolved legacy HKL channel, an adoptable IOC record, or an HDF5 axis dataset remove: the six regression tests that existed only to pin the removed contract, plus the LABEL-fallback special case in the three-way merge that stopped a blank field from reading as a live caput
… the header tuple still had six entries against five axis_keys, so every column after Label was mislabelled (a motor typed under "Record name" was stored as SOURCE_PV) and the Units column was never read
Osayi-ANL
left a comment
Osayi-ANL
left a comment
There was a problem hiding this comment.
I think it's good to merge
add: Clear button on the log viewer, plus a Follow toggle so the window keeps up with new lines while it is open and opens on the newest one
change: HpcAdMetadataProcessor and HpcPassthroughProcessor to inherit from the hpc_metadata base
…and analysis consumers share them from a neutral package instead of under hpc/ add: class docstrings with runnable usage examples to BaseHpcProcessor and BaseMetaAssociator
…pcProcessor providing image/ROI scaffolding and NDAttribute parsing for analysis consumers change: refactor RSM, vectorized, and spontaneous analysis consumers to extend BaseAnalysisProcessor, dropping duplicated stats, codec maps, (de)compression helpers, and stats hooks
…its duplicated codec map, union-field dtype map, generic stats init, and log setup while keeping the CA-monitor MCA matching and custom nMcaWithin/nMcaStale stats Fixes #99
…og with its traceback instead of the workflow console box, where it scrolls away and cannot be searched afterwards add: BaseHpcProcessor.start prints a flushed startup line naming the processor, since the console box is where an operator looks after hitting Start and a consumer can sit a while before its first frame change: the RSM and spontaneous consumers report through log_error, replacing two bare prints and the hasattr(self, 'logger') guards that silently swallowed failures
… on the first configure()/process() — matching is windowed at 0.5s, so readings had to already be in hand when a frame arrived and early frames could never match add: one-shot MCA diagnostics — monitor startup with PV names, the first reading per channel, a frame that found no readings, and the first stale reading with its delta — kept to first-occurrence only since the per-frame counters already report the rest add: a console line when association actually begins, naming the matched channels, so silence after startup means nothing matched rather than being ambiguous
…sh handling lives in one place and no processor calls print directly change: the startup wording lives on the base class with a startup_details hook for per-processor detail, instead of each processor writing its own line change: the vectorized consumer's stray "First Scan detected" print goes through announce and names the processor that said it
…rocessor file from SIM_SERVER_TYPES, plus a probe-shape dropdown shown only when the probe server is selected fix: each simulation is launched with only the flags its own parser accepts — the probe server rejects -nf and -mpv, and the RSM data server takes no arguments at all, so both would have exited on argparse before starting
…ne summary — consumer id, channel, receive/publish rates, frame counts, and the rsm_grid accumulation state — coloured by whether frames are landing, matching the treatment the associator output already gets change: the analysis worker's output goes through the formatter instead of appendPlainText, so a stats report no longer pushes everything worth reading off screen
Extract shared HPC processor base classes into consumers/core
2 participants
Stack 4 of 4. Base
rsm/03-detector-physics. This branch contains the whole stack — check this one out for beamline testing and record its SHA in the test notes.Draft until validated on real PVs. Three reviewable commits.
Why
Live 3D today is a point cloud with amnesia: a 100-frame, 1M-point ring buffer that overwrites its oldest frame. Scan a region twice and the second pass evicts the first rather than reinforcing it. PR #129 gave us the accumulator concept offline; this brings it live, so a voxel means the same thing during the scan as in the file you reopen later.
Commit 1 — Qt-free volume persistence
The accumulator runs in a pvaccess consumer, and this repo documents that mixing pvaccess with PyQt5 core-dumps (the two-process note in
consumers/ioc_rsm_parameter). So the write path had to lose Qt first.volume_io.pyimports no Qt and produces the same layout Workbench already reads.HDF5Writerbecomes a plain class with anon_finishedcallback;HDF5Handlerre-wraps it as the signal — the split its own docstring already described.hkl_3d_viewerandarea_det_viewerusedHDF5Writerdirectly as a threaded QObject and now useHDF5Handler.log_managerwas pulling PyQt5 into everything viaLogMixinfor one type annotation and one.instance()call; both deferred. A subprocess test asserts the write path stays Qt-free.Empty voxels become NaN with a coverage volume alongside —
Gridder3Dleaves un-hit bins at zero, indistinguishable from a measured zero.volume_result_to_metadatausedvolume.min()/max(), which would have returned[nan, nan]the moment empty voxels became NaN, breaking every colour scale that trusts it; now finite-only.Coverage does not make a flux-weighted mean recoverable: from
sum(I/m)andNyou cannot recoversum(I)andsum(m)separately unless the monitor was constant across a voxel's contributions.gridder_access.pyis the one place touchingGridder3D._gdata/_gnorm, avoiding.data(which copies the whole numerator and divides on every access — 134 MB per snapshot at 256³). Its guard pins the semantics, sum and count, not just that the attributes exist.Commit 2 — Fail-closed metadata binding
If a frame's angles belong to a different frame, every pixel lands in the wrong place and the result still looks like a diffraction volume. There is no visual tell. So a frame whose required metadata is missing, stale, non-finite or duplicated is rejected and counted, never gridded with a substituted value.
Channels are classified: STATIC geometry legitimately arrives once and is latched; REQUIRED_DYNAMIC (circle positions, monitor) must be fresh; OPTIONAL never blocks. Rejections are counted per reason, so "the preview is empty" resolves to a cause. Scope is frame-bound and trigger-latched metadata only — fly-scan interpolation and pulse-ID joins are deliberately not attempted, because guessing at them is the exact mis-binding this prevents.
Commit 3 — Accumulator, session, consumer, dock
Bounds are locked before the first frame:
Gridder3Dlatchesfixed_rangeon first use, and rebinning mid-scan would change what a voxel means. Changing bounds needs a new accumulator —Clear()keeps the latched range.Two grids over the same accepted samples: the full-resolution one that gets saved, and an independently gridded coarse one for the preview. One extra C call per frame, versus sweeping hundreds of MB of accumulator every snapshot in the process running the hot loop. Coarse shape is chosen per axis to fit 4 MiB while preserving aspect ratio.
Aggregation matches the offline builder exactly — unweighted mean, monitor-normalized per frame. Summing would brighten voxels wherever the scan path sampled more often, an artifact of the trajectory, not the scattering.
Nothing drops silently: out-of-range, non-finite and masked samples are counted separately and shown on screen. Masked pixels are excluded, not zeroed.
HpcRsmGridProcessoris an alternative toHpcRsmProcessor, not an addition — that one attaches three float64 Q arrays to every frame (~100 MB/frame at 2048²); this computes Q where it is consumed and publishes only the preview. It subclasses rather than forks the Q path. Single-instance by nature, so drops are counted and the preview marked incomplete: a drop-aware preview, not a lossless record. Full-resolution state never crosses PVA; saving happens in the consumer, confined toOUTPUT_PATH, refusing overwrite, and only fromstoppedso it cannot capture a torn accumulator.The HKL3D dock adds bounds/resolution, Estimate/Start/Stop/Clear/Save and live counters. Bounds lock while running. Estimate is labelled as observed bounds — later scan motion can still fall outside, which is why the out-of-grid count sits beside it. Uncovered voxels render transparent.
The assertion that matters
Live and offline agree for identical frames, bounds and resolution — coverage exactly equal, means to 1e-12.
build_volumegained an optionalfixed_boundsso the comparison is possible at all.444 tests pass (227 at #129),
ruff check src/dashpva/clean.Blocked on beam
metadata/ca/key is I0Known limits
Single consumer caps throughput; no mid-acquisition snapshot (save requires stop); no full-volume transfer over PVA; assumes shared storage visible to both the consumer and Workbench.