Expose per-peak metrics errors via CLI (CSV/histograms) - #9
Draft
PrinceWalnut wants to merge 7 commits into
Draft
Conversation
Adds return_per_peak option to compute_metrics plus a CSV writer and histogram plotting helpers, so the raw error distributions can be inspected instead of only the aggregated median/mean/max stats. Wired up as --csv and --plot options on the `subhkl metrics` CLI command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UsgjQGXqZVZBYqxoXvPTwk
Splits each into a bool flag plus a --csv-path/--plot-path override. Click's is_flag=False/flag_value optional-argument trick was tested against the exact typer==0.21.1/click==8.3.1 pinned in uv.lock and errors with "Option '--csv' requires an argument", so a single --csv <path> option can't support "no path" reliably on this stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UsgjQGXqZVZBYqxoXvPTwk
compute_metrics conflated the two: for Predictor "banks" output, the value used as "run_index" throughout was actually the composite frame+panel slot key, so --per-run grouped by panel rather than by true rotation frame whenever a frame had multiple detector panels. extract_xyz_from_file now returns panel_index (physical bank) and frame_index (true frame) alongside the existing slot index used internally for goniometer/angles and goniometer/R lookups (left untouched since those arrays are stored per-slot). frame_index is read from peaks/run_index directly for Indexer/Integrator-format files, and reconstructed for Predictor output by grouping slots that share an identical goniometer/angles row (those rows are tiled per-panel from a single frame's angles at write time, confirmed via source trace). --per-run now groups by true frame; the per-peak CSV gains a panel_index column alongside frame_index; the per-run histogram plot (renamed plot_per_frame_histograms) facets by true frame instead of panel/slot. Verified against synthetic HDF5 fixtures reproducing the frame/panel tiling pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UsgjQGXqZVZBYqxoXvPTwk
plot_error_histograms/plot_per_frame_histograms gain a bins parameter (default 50/30, matching the previously hardcoded values), threaded through run_metrics as plot_bins and exposed on the CLI as --bins. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UsgjQGXqZVZBYqxoXvPTwk
run_index() unconditionally overwrote the true per-peak frame id (peaks/run_index, correctly written upstream by Peaks.write_hdf5) with the composite frame+panel slot id (peaks/image_index) whenever the latter was present in the loaded input data - which is essentially always for Finder output. This corrupted frame_index downstream (e.g. in subhkl metrics --per-run) to span n_frames * n_panels instead of n_frames, and fed the same corrupted value into FindUB.load_from_dict, which then wrote it right back into the indexed output file. Only fall back to image_index when run_index is genuinely absent. Files already indexed with the old behavior need to be re-indexed to pick up correct peaks/run_index values. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UsgjQGXqZVZBYqxoXvPTwk
compute_metrics now unpacks (xyz, slot_index, panel_index, frame_index) from extract_xyz_from_file, but this test's mock still returned the old 2-tuple. The resulting unpack ValueError was silently caught and masked by run_metrics' "METRICS: 9.99 ..." exception sentinel, so the test was comparing against a fake fallback value instead of the real median angular error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UsgjQGXqZVZBYqxoXvPTwk
Collaborator
vivek-booshan
approved these changes
Jul 29, 2026
Collaborator
|
I am not aware of PR #9 implementation, and so far I know it does not work and badly affects reproducibility. PR #11 is for error analysis and a separate implementation. Thanks, @jglaser and @vivek-booshan |
Collaborator
|
@kbega Can you please explain what do you mean by "does not work"? |
PrinceWalnut
marked this pull request as draft
July 30, 2026 23:15
Fixed linting issues.
SmithRWORNL
approved these changes
Aug 11, 2026
Collaborator
|
Thanks! @SmithRWORNL @kbega can you please check for redundancy with #38 |
Contributor
|
My non-expert eye couldn't identify any code that looked like it was doing the same thing in both branches. The comparison, for reference: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
compute_metricsgains areturn_per_peakoption that attaches the raw per-peakrun_index/h/k/l/d_err/ang_errarrays to its result, instead of only aggregated median/mean/max stats.write_per_peak_csvand a newsubhkl.viz.metrics_plotsmodule (plot_error_histograms,plot_per_run_histograms) so those distributions can be written to CSV or plotted as histograms (optionally faceted per run/frame via--per-run).subhkl metricsCLI command as--csv/--csv-pathand--plot/--plot-path. The bare flags default tometrics.csv/metrics.png; the-pathvariants override the filename. (A single--csv <path>option was tried first but doesn't reliably support "flag with no value" against thetyper/clickversions pinned inuv.lock, hence the split.)Test plan
subhkl metrics indexer.h5 --csv --plotwritesmetrics.csv/metrics.pngsubhkl metrics indexer.h5 --csv-path errors.csv --plot-path errors.pngwrites the custom-named filessubhkl metrics indexer.h5 --per-run --plotadditionally writesmetrics_per_run.pngwith one histogram row per framesubhkl metricsinvocations without these flags behave unchanged🤖 Generated with Claude Code
https://claude.ai/code/session_01UsgjQGXqZVZBYqxoXvPTwk