Skip to content

Commit ca825c4

Browse files
Fix powder chart y-axis range and clarify planning workflow (#166)
* Add powder chart y-range fixing plan * Fix powder chart y-axis range * Clarify planning workflow instructions
1 parent b955261 commit ca825c4

4 files changed

Lines changed: 210 additions & 13 deletions

File tree

‎.github/copilot-instructions.md‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -171,10 +171,23 @@ When asked to create a plan:
171171
- Apply the two-phase workflow (Phase 1 implementation, Phase 2
172172
verification) to non-trivial plans. Stop after Phase 1 and ask the
173173
user to review before starting Phase 2.
174-
- Every completed implementation step ends with a local commit following
175-
the rules in **Commits**. Keep commits atomic, single-purpose, and
176-
aligned with plan steps.
174+
- The plan must explicitly state that, when an AI agent follows it,
175+
every completed Phase 1 implementation step must be staged with
176+
explicit paths and committed locally before moving to the next
177+
implementation step or the Phase 1 review gate. Follow the rules in
178+
**Commits**. Keep commits atomic, single-purpose, and aligned with
179+
plan steps.
180+
- If implementation uncovers a serious requirement, risk, design issue,
181+
or scope change not covered by the plan, stop and ask the user for
182+
clarification or approval before proceeding. Record the unresolved
183+
issue in the plan when useful.
177184
- The plan should be easy to maintain while working: include concrete
178185
files likely to change, decisions already made, open questions,
179186
verification commands for Phase 2, and a short suggested commit
180187
message or branch name when useful.
188+
- End every plan with a "Suggested Pull Request" section containing a
189+
short PR title and a brief end-user-oriented description. Keep this
190+
section non-technical enough for scientists and other users to
191+
understand the benefit. Update it during implementation if extra
192+
approved changes become important enough to mention in the PR title or
193+
description.
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
# Powder Chart Y-Range Fix Plan
2+
3+
**Date:** 2026-05-06 **Status:** Phase 2 verified — complete
4+
5+
---
6+
7+
## 1. Goal
8+
9+
Fix the Plotly composite powder measured-vs-calculated chart so the main
10+
intensity row is not anchored to zero. The y-axis range should be
11+
derived from all displayed main-row intensity series: measured
12+
(`Imeas`), calculated (`Icalc`), and background (`Ibkg`) when present.
13+
14+
The intended display range is:
15+
16+
```text
17+
lower = min(Imeas, Icalc, Ibkg) - margin
18+
upper = max(Imeas, Icalc, Ibkg) + margin
19+
```
20+
21+
where `margin` is controlled by a dedicated constant of about 5% of the
22+
main intensity span. The lower bound must use `min - margin`, not
23+
`min + margin`, so the lowest displayed point remains visible with
24+
padding below it.
25+
26+
---
27+
28+
## 2. Current Findings
29+
30+
- The affected code is `PlotlyPlotter._get_main_intensity_range()` in
31+
`src/easydiffraction/display/plotters/plotly.py`.
32+
- It currently uses only `y_meas` and `y_calc`, then forces
33+
`lower_limit = min(0.0, main_y_min)`. That explains positive powder
34+
charts being truncated to a `0..max` range.
35+
- `PowderMeasVsCalcSpec` already carries optional `y_bkg`, and
36+
`Plotter._plot_meas_vs_calc_data()` already filters
37+
`pattern.intensity_bkg` into the spec for powder Bragg plots.
38+
- Existing Plotly unit tests assert the current `0.0..max` y-range in
39+
the residual scale-match tests, so those expectations must change.
40+
- Repository memory notes confirm the background line is part of the
41+
composite powder plot and should be considered display data.
42+
43+
---
44+
45+
## 3. Scope
46+
47+
### In Scope
48+
49+
- Add a module-level constant in
50+
`src/easydiffraction/display/plotters/plotly.py`, likely
51+
`MAIN_INTENSITY_RANGE_MARGIN_FRACTION = 0.05`.
52+
- Update `_get_main_intensity_range()` to compute min/max over `y_meas`,
53+
`y_calc`, and non-empty `y_bkg` when present.
54+
- Apply symmetric visual padding outside the data range using the new
55+
constant.
56+
- Preserve the existing empty-filtered-range behavior: empty required
57+
series should still return a harmless fallback range.
58+
- Preserve residual scale matching by letting `_get_residual_limit()`
59+
use the newly padded main range and the existing
60+
`residual_height_fraction`, so the residual row remains adjusted to
61+
the main row size as it is now.
62+
- Add/update focused unit tests for range calculation and affected
63+
residual-scale expectations.
64+
65+
### Out of Scope
66+
67+
- No public plotting API changes.
68+
- No user-configurable y-axis margin in this step.
69+
- No changes to ASCII plotting unless a later review shows the same main
70+
view problem exists there.
71+
- No refactor of plot layout, Bragg tick sizing, hover templates, or
72+
facade routing.
73+
74+
---
75+
76+
## 4. Decisions
77+
78+
- Use `min(Imeas, Icalc, Ibkg) - margin` for the lower y-axis bound and
79+
`max(Imeas, Icalc, Ibkg) + margin` for the upper y-axis bound.
80+
- Keep the residual plot scaled to the main intensity row, preserving
81+
the current matched-scale behavior after the main range gains padding.
82+
83+
---
84+
85+
## 5. Implementation Checklist
86+
87+
- [ ] Create branch `feature/powder-chart-y-range` if requested.
88+
- [x] In `src/easydiffraction/display/plotters/plotly.py`, add the
89+
dedicated 5% y-range margin constant near the other Plotly layout
90+
constants.
91+
- [x] Update `_get_main_intensity_range()` so it includes background
92+
intensity when available and uses the padded min/max range instead
93+
of anchoring positive data to zero.
94+
- [x] Keep zero-span data explicit and stable, using a small fallback
95+
range around the datum because a percentage margin is undefined.
96+
- [x] Confirm `_get_residual_limit()` continues to scale the residual
97+
row from the updated main y-range and existing residual height
98+
fraction.
99+
- [x] Stop after Phase 1 and request review before adding or running
100+
tests, following the repo workflow.
101+
102+
---
103+
104+
## 6. Phase 2 Verification Checklist
105+
106+
- [x] Add or update tests in
107+
`tests/unit/easydiffraction/display/plotters/test_plotly.py` for:
108+
- positive-only `Imeas`/`Icalc` data no longer starting at zero;
109+
- `Ibkg` lowering or raising the main y-range when present;
110+
- 5% padding on both ends of the main row;
111+
- residual scale-match expectations after padding changes the main row
112+
span;
113+
- empty filtered arrays retaining the existing fallback behavior.
114+
- [x] Keep the existing facade propagation test in
115+
`tests/unit/easydiffraction/display/test_plotting.py` unless the
116+
implementation reveals a missing background handoff case.
117+
- [x] Run `pixi run fix`.
118+
- [x] Run `pixi run check` until clean.
119+
- [x] Run `pixi run unit-tests`.
120+
- [x] Run `pixi run integration-tests`.
121+
- [x] Run `pixi run script-tests`.
122+
123+
---
124+
125+
## 7. Likely Files
126+
127+
- `src/easydiffraction/display/plotters/plotly.py`
128+
- `tests/unit/easydiffraction/display/plotters/test_plotly.py`
129+
- `tests/unit/easydiffraction/display/test_plotting.py` only if a
130+
facade-level test gap is discovered during verification.
131+
132+
---
133+
134+
## 8. Suggested Commit Message
135+
136+
```text
137+
Fix powder chart y-axis range
138+
```

‎src/easydiffraction/display/plotters/plotly.py‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
BRAGG_TICK_MARKER_SIZE = 12
6161
BRAGG_TICK_MARKER_LINE_WIDTH = 1
6262
BRAGG_TICK_SYMBOL_HEIGHT_SCALE = 1.4
63+
MAIN_INTENSITY_RANGE_MARGIN_FRACTION = 0.05
6364
COMPOSITE_VERTICAL_SPACING = 0.03
6465
COMPOSITE_MARGIN_RIGHT = 30
6566
COMPOSITE_MARGIN_TOP = 40
@@ -910,12 +911,20 @@ def _get_main_intensity_range(cls, plot_spec: PowderMeasVsCalcSpec) -> tuple[flo
910911
if min(y_meas.size, y_calc.size) == 0:
911912
return 0.0, 1.0
912913

913-
main_y_min = float(min(np.min(y_meas), np.min(y_calc)))
914-
main_y_max = float(max(np.max(y_meas), np.max(y_calc)))
915-
lower_limit = min(0.0, main_y_min)
916-
if main_y_max <= lower_limit:
917-
return lower_limit - 1.0, lower_limit + 1.0
918-
return lower_limit, main_y_max
914+
main_series = [y_meas, y_calc]
915+
if plot_spec.y_bkg is not None:
916+
y_bkg = np.asarray(plot_spec.y_bkg)
917+
if y_bkg.size > 0:
918+
main_series.append(y_bkg)
919+
920+
main_y_min = float(min(np.min(series) for series in main_series))
921+
main_y_max = float(max(np.max(series) for series in main_series))
922+
main_y_range = main_y_max - main_y_min
923+
if main_y_range > 0.0:
924+
main_y_margin = main_y_range * MAIN_INTENSITY_RANGE_MARGIN_FRACTION
925+
return main_y_min - main_y_margin, main_y_max + main_y_margin
926+
927+
return main_y_min - 1.0, main_y_max + 1.0
919928

920929
@classmethod
921930
def _get_residual_limit(cls, plot_spec: PowderMeasVsCalcSpec) -> float:

‎tests/unit/easydiffraction/display/plotters/test_plotly.py‎

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -400,13 +400,40 @@ def fake_show_figure(self, fig):
400400
assert background_trace.mode == 'lines'
401401
assert background_trace.line.color == pp.DEFAULT_COLORS['bkg']
402402
assert background_trace.line.width == pp.BACKGROUND_LINE_WIDTH
403+
raw_min = 1.5
404+
raw_max = 12.0
405+
raw_range = raw_max - raw_min
406+
margin = raw_range * pp.MAIN_INTENSITY_RANGE_MARGIN_FRACTION
407+
assert fig.layout.yaxis.range[0] == pytest.approx(raw_min - margin)
408+
assert fig.layout.yaxis.range[1] == pytest.approx(raw_max + margin)
403409
assert meas_trace.legendrank < background_trace.legendrank < calc_trace.legendrank
404410
assert residual_trace.legendrank > calc_trace.legendrank
405411
for trace in (meas_trace, background_trace, calc_trace, residual_trace):
406412
assert 'Ibkg: %{customdata[1]' in trace.hovertemplate
407413
assert list(trace.customdata[0]) == pytest.approx([10.0, 1.5, 9.0, 1.0])
408414

409415

416+
def test_get_main_intensity_range_uses_unit_padding_for_flat_series():
417+
from easydiffraction.display.plotters.base import PowderMeasVsCalcSpec
418+
from easydiffraction.display.plotters.plotly import PlotlyPlotter
419+
420+
plot_spec = PowderMeasVsCalcSpec(
421+
x=np.array([1.0]),
422+
y_meas=np.array([5.0]),
423+
y_calc=np.array([5.0]),
424+
y_resid=None,
425+
bragg_tick_sets=(),
426+
axes_labels=['2θ (degree)', 'Intensity (arb. units)'],
427+
title='Powder',
428+
residual_height_fraction=0.25,
429+
bragg_peaks_height_fraction=0.10,
430+
height=None,
431+
y_bkg=np.array([5.0]),
432+
)
433+
434+
assert PlotlyPlotter._get_main_intensity_range(plot_spec) == pytest.approx((4.0, 6.0))
435+
436+
410437
def test_bragg_row_height_pixels_scale_linearly_with_phase_count():
411438
from easydiffraction.display.plotters.base import BraggTickSet
412439
from easydiffraction.display.plotters.base import PowderMeasVsCalcSpec
@@ -639,11 +666,17 @@ def fake_show_figure(self, fig):
639666
)
640667

641668
fig = captured['fig']
642-
expected_limit = 0.5 * (3600.0 - 0.0) * 0.25
669+
raw_min = 180.0
670+
raw_max = 3600.0
671+
raw_range = raw_max - raw_min
672+
margin = raw_range * pp.MAIN_INTENSITY_RANGE_MARGIN_FRACTION
673+
expected_main_min = raw_min - margin
674+
expected_main_max = raw_max + margin
675+
expected_limit = 0.5 * (expected_main_max - expected_main_min) * 0.25
643676
assert fig.layout.yaxis2.scaleanchor == 'y'
644677
assert fig.layout.yaxis2.scaleratio == pytest.approx(1.0)
645-
assert fig.layout.yaxis.range[0] == pytest.approx(0.0)
646-
assert fig.layout.yaxis.range[1] == pytest.approx(3600.0)
678+
assert fig.layout.yaxis.range[0] == pytest.approx(expected_main_min)
679+
assert fig.layout.yaxis.range[1] == pytest.approx(expected_main_max)
647680
assert fig.layout.yaxis2.range[0] == pytest.approx(-expected_limit)
648681
assert fig.layout.yaxis2.range[1] == pytest.approx(expected_limit)
649682
plot_area_height = fig.layout.height - fig.layout.margin.t - fig.layout.margin.b
@@ -688,7 +721,11 @@ def fake_show_figure(self, fig):
688721
)
689722

690723
fig = captured['fig']
691-
expected_limit = 0.5 * (3600.0 - 0.0) * 0.25
724+
raw_min = 180.0
725+
raw_max = 3600.0
726+
raw_range = raw_max - raw_min
727+
margin = raw_range * pp.MAIN_INTENSITY_RANGE_MARGIN_FRACTION
728+
expected_limit = 0.5 * ((raw_max + margin) - (raw_min - margin)) * 0.25
692729
assert fig.layout.yaxis2.range[0] == pytest.approx(-expected_limit)
693730
assert fig.layout.yaxis2.range[1] == pytest.approx(expected_limit)
694731
assert list(fig.layout.yaxis2.tickvals) == pytest.approx([-400.0, 0.0, 400.0])

0 commit comments

Comments
 (0)