diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 93b84d2eb..18b01c478 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -171,10 +171,23 @@ When asked to create a plan: - Apply the two-phase workflow (Phase 1 implementation, Phase 2 verification) to non-trivial plans. Stop after Phase 1 and ask the user to review before starting Phase 2. -- Every completed implementation step ends with a local commit following - the rules in **Commits**. Keep commits atomic, single-purpose, and - aligned with plan steps. +- The plan must explicitly state that, when an AI agent follows it, + every completed Phase 1 implementation step must be staged with + explicit paths and committed locally before moving to the next + implementation step or the Phase 1 review gate. Follow the rules in + **Commits**. Keep commits atomic, single-purpose, and aligned with + plan steps. +- If implementation uncovers a serious requirement, risk, design issue, + or scope change not covered by the plan, stop and ask the user for + clarification or approval before proceeding. Record the unresolved + issue in the plan when useful. - The plan should be easy to maintain while working: include concrete files likely to change, decisions already made, open questions, verification commands for Phase 2, and a short suggested commit message or branch name when useful. +- End every plan with a "Suggested Pull Request" section containing a + short PR title and a brief end-user-oriented description. Keep this + section non-technical enough for scientists and other users to + understand the benefit. Update it during implementation if extra + approved changes become important enough to mention in the PR title or + description. diff --git a/docs/dev/plan_powder-chart-y-range.md b/docs/dev/plan_powder-chart-y-range.md new file mode 100644 index 000000000..c987fe47a --- /dev/null +++ b/docs/dev/plan_powder-chart-y-range.md @@ -0,0 +1,138 @@ +# Powder Chart Y-Range Fix Plan + +**Date:** 2026-05-06 **Status:** Phase 2 verified — complete + +--- + +## 1. Goal + +Fix the Plotly composite powder measured-vs-calculated chart so the main +intensity row is not anchored to zero. The y-axis range should be +derived from all displayed main-row intensity series: measured +(`Imeas`), calculated (`Icalc`), and background (`Ibkg`) when present. + +The intended display range is: + +```text +lower = min(Imeas, Icalc, Ibkg) - margin +upper = max(Imeas, Icalc, Ibkg) + margin +``` + +where `margin` is controlled by a dedicated constant of about 5% of the +main intensity span. The lower bound must use `min - margin`, not +`min + margin`, so the lowest displayed point remains visible with +padding below it. + +--- + +## 2. Current Findings + +- The affected code is `PlotlyPlotter._get_main_intensity_range()` in + `src/easydiffraction/display/plotters/plotly.py`. +- It currently uses only `y_meas` and `y_calc`, then forces + `lower_limit = min(0.0, main_y_min)`. That explains positive powder + charts being truncated to a `0..max` range. +- `PowderMeasVsCalcSpec` already carries optional `y_bkg`, and + `Plotter._plot_meas_vs_calc_data()` already filters + `pattern.intensity_bkg` into the spec for powder Bragg plots. +- Existing Plotly unit tests assert the current `0.0..max` y-range in + the residual scale-match tests, so those expectations must change. +- Repository memory notes confirm the background line is part of the + composite powder plot and should be considered display data. + +--- + +## 3. Scope + +### In Scope + +- Add a module-level constant in + `src/easydiffraction/display/plotters/plotly.py`, likely + `MAIN_INTENSITY_RANGE_MARGIN_FRACTION = 0.05`. +- Update `_get_main_intensity_range()` to compute min/max over `y_meas`, + `y_calc`, and non-empty `y_bkg` when present. +- Apply symmetric visual padding outside the data range using the new + constant. +- Preserve the existing empty-filtered-range behavior: empty required + series should still return a harmless fallback range. +- Preserve residual scale matching by letting `_get_residual_limit()` + use the newly padded main range and the existing + `residual_height_fraction`, so the residual row remains adjusted to + the main row size as it is now. +- Add/update focused unit tests for range calculation and affected + residual-scale expectations. + +### Out of Scope + +- No public plotting API changes. +- No user-configurable y-axis margin in this step. +- No changes to ASCII plotting unless a later review shows the same main + view problem exists there. +- No refactor of plot layout, Bragg tick sizing, hover templates, or + facade routing. + +--- + +## 4. Decisions + +- Use `min(Imeas, Icalc, Ibkg) - margin` for the lower y-axis bound and + `max(Imeas, Icalc, Ibkg) + margin` for the upper y-axis bound. +- Keep the residual plot scaled to the main intensity row, preserving + the current matched-scale behavior after the main range gains padding. + +--- + +## 5. Implementation Checklist + +- [ ] Create branch `feature/powder-chart-y-range` if requested. +- [x] In `src/easydiffraction/display/plotters/plotly.py`, add the + dedicated 5% y-range margin constant near the other Plotly layout + constants. +- [x] Update `_get_main_intensity_range()` so it includes background + intensity when available and uses the padded min/max range instead + of anchoring positive data to zero. +- [x] Keep zero-span data explicit and stable, using a small fallback + range around the datum because a percentage margin is undefined. +- [x] Confirm `_get_residual_limit()` continues to scale the residual + row from the updated main y-range and existing residual height + fraction. +- [x] Stop after Phase 1 and request review before adding or running + tests, following the repo workflow. + +--- + +## 6. Phase 2 Verification Checklist + +- [x] Add or update tests in + `tests/unit/easydiffraction/display/plotters/test_plotly.py` for: + - positive-only `Imeas`/`Icalc` data no longer starting at zero; + - `Ibkg` lowering or raising the main y-range when present; + - 5% padding on both ends of the main row; + - residual scale-match expectations after padding changes the main row + span; + - empty filtered arrays retaining the existing fallback behavior. +- [x] Keep the existing facade propagation test in + `tests/unit/easydiffraction/display/test_plotting.py` unless the + implementation reveals a missing background handoff case. +- [x] Run `pixi run fix`. +- [x] Run `pixi run check` until clean. +- [x] Run `pixi run unit-tests`. +- [x] Run `pixi run integration-tests`. +- [x] Run `pixi run script-tests`. + +--- + +## 7. Likely Files + +- `src/easydiffraction/display/plotters/plotly.py` +- `tests/unit/easydiffraction/display/plotters/test_plotly.py` +- `tests/unit/easydiffraction/display/test_plotting.py` only if a + facade-level test gap is discovered during verification. + +--- + +## 8. Suggested Commit Message + +```text +Fix powder chart y-axis range +``` diff --git a/src/easydiffraction/display/plotters/plotly.py b/src/easydiffraction/display/plotters/plotly.py index 3072b142d..e3d9e4e3a 100644 --- a/src/easydiffraction/display/plotters/plotly.py +++ b/src/easydiffraction/display/plotters/plotly.py @@ -60,6 +60,7 @@ BRAGG_TICK_MARKER_SIZE = 12 BRAGG_TICK_MARKER_LINE_WIDTH = 1 BRAGG_TICK_SYMBOL_HEIGHT_SCALE = 1.4 +MAIN_INTENSITY_RANGE_MARGIN_FRACTION = 0.05 COMPOSITE_VERTICAL_SPACING = 0.03 COMPOSITE_MARGIN_RIGHT = 30 COMPOSITE_MARGIN_TOP = 40 @@ -910,12 +911,20 @@ def _get_main_intensity_range(cls, plot_spec: PowderMeasVsCalcSpec) -> tuple[flo if min(y_meas.size, y_calc.size) == 0: return 0.0, 1.0 - main_y_min = float(min(np.min(y_meas), np.min(y_calc))) - main_y_max = float(max(np.max(y_meas), np.max(y_calc))) - lower_limit = min(0.0, main_y_min) - if main_y_max <= lower_limit: - return lower_limit - 1.0, lower_limit + 1.0 - return lower_limit, main_y_max + main_series = [y_meas, y_calc] + if plot_spec.y_bkg is not None: + y_bkg = np.asarray(plot_spec.y_bkg) + if y_bkg.size > 0: + main_series.append(y_bkg) + + main_y_min = float(min(np.min(series) for series in main_series)) + main_y_max = float(max(np.max(series) for series in main_series)) + main_y_range = main_y_max - main_y_min + if main_y_range > 0.0: + main_y_margin = main_y_range * MAIN_INTENSITY_RANGE_MARGIN_FRACTION + return main_y_min - main_y_margin, main_y_max + main_y_margin + + return main_y_min - 1.0, main_y_max + 1.0 @classmethod def _get_residual_limit(cls, plot_spec: PowderMeasVsCalcSpec) -> float: diff --git a/tests/unit/easydiffraction/display/plotters/test_plotly.py b/tests/unit/easydiffraction/display/plotters/test_plotly.py index 6f43d2221..20cff4903 100644 --- a/tests/unit/easydiffraction/display/plotters/test_plotly.py +++ b/tests/unit/easydiffraction/display/plotters/test_plotly.py @@ -400,6 +400,12 @@ def fake_show_figure(self, fig): assert background_trace.mode == 'lines' assert background_trace.line.color == pp.DEFAULT_COLORS['bkg'] assert background_trace.line.width == pp.BACKGROUND_LINE_WIDTH + raw_min = 1.5 + raw_max = 12.0 + raw_range = raw_max - raw_min + margin = raw_range * pp.MAIN_INTENSITY_RANGE_MARGIN_FRACTION + assert fig.layout.yaxis.range[0] == pytest.approx(raw_min - margin) + assert fig.layout.yaxis.range[1] == pytest.approx(raw_max + margin) assert meas_trace.legendrank < background_trace.legendrank < calc_trace.legendrank assert residual_trace.legendrank > calc_trace.legendrank for trace in (meas_trace, background_trace, calc_trace, residual_trace): @@ -407,6 +413,27 @@ def fake_show_figure(self, fig): assert list(trace.customdata[0]) == pytest.approx([10.0, 1.5, 9.0, 1.0]) +def test_get_main_intensity_range_uses_unit_padding_for_flat_series(): + from easydiffraction.display.plotters.base import PowderMeasVsCalcSpec + from easydiffraction.display.plotters.plotly import PlotlyPlotter + + plot_spec = PowderMeasVsCalcSpec( + x=np.array([1.0]), + y_meas=np.array([5.0]), + y_calc=np.array([5.0]), + y_resid=None, + bragg_tick_sets=(), + axes_labels=['2θ (degree)', 'Intensity (arb. units)'], + title='Powder', + residual_height_fraction=0.25, + bragg_peaks_height_fraction=0.10, + height=None, + y_bkg=np.array([5.0]), + ) + + assert PlotlyPlotter._get_main_intensity_range(plot_spec) == pytest.approx((4.0, 6.0)) + + def test_bragg_row_height_pixels_scale_linearly_with_phase_count(): from easydiffraction.display.plotters.base import BraggTickSet from easydiffraction.display.plotters.base import PowderMeasVsCalcSpec @@ -639,11 +666,17 @@ def fake_show_figure(self, fig): ) fig = captured['fig'] - expected_limit = 0.5 * (3600.0 - 0.0) * 0.25 + raw_min = 180.0 + raw_max = 3600.0 + raw_range = raw_max - raw_min + margin = raw_range * pp.MAIN_INTENSITY_RANGE_MARGIN_FRACTION + expected_main_min = raw_min - margin + expected_main_max = raw_max + margin + expected_limit = 0.5 * (expected_main_max - expected_main_min) * 0.25 assert fig.layout.yaxis2.scaleanchor == 'y' assert fig.layout.yaxis2.scaleratio == pytest.approx(1.0) - assert fig.layout.yaxis.range[0] == pytest.approx(0.0) - assert fig.layout.yaxis.range[1] == pytest.approx(3600.0) + assert fig.layout.yaxis.range[0] == pytest.approx(expected_main_min) + assert fig.layout.yaxis.range[1] == pytest.approx(expected_main_max) assert fig.layout.yaxis2.range[0] == pytest.approx(-expected_limit) assert fig.layout.yaxis2.range[1] == pytest.approx(expected_limit) plot_area_height = fig.layout.height - fig.layout.margin.t - fig.layout.margin.b @@ -688,7 +721,11 @@ def fake_show_figure(self, fig): ) fig = captured['fig'] - expected_limit = 0.5 * (3600.0 - 0.0) * 0.25 + raw_min = 180.0 + raw_max = 3600.0 + raw_range = raw_max - raw_min + margin = raw_range * pp.MAIN_INTENSITY_RANGE_MARGIN_FRACTION + expected_limit = 0.5 * ((raw_max + margin) - (raw_min - margin)) * 0.25 assert fig.layout.yaxis2.range[0] == pytest.approx(-expected_limit) assert fig.layout.yaxis2.range[1] == pytest.approx(expected_limit) assert list(fig.layout.yaxis2.tickvals) == pytest.approx([-400.0, 0.0, 400.0])