Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
138 changes: 138 additions & 0 deletions docs/dev/plan_powder-chart-y-range.md
Original file line number Diff line number Diff line change
@@ -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
```
21 changes: 15 additions & 6 deletions src/easydiffraction/display/plotters/plotly.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
45 changes: 41 additions & 4 deletions tests/unit/easydiffraction/display/plotters/test_plotly.py
Original file line number Diff line number Diff line change
Expand Up @@ -400,13 +400,40 @@ 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):
assert 'Ibkg: %{customdata[1]' in trace.hovertemplate
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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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])
Expand Down
Loading