Skip to content

Centre the oversampled model grid on the detector pixels - #160

Closed
almilder wants to merge 3 commits into
mainfrom
fix/pixel-centered-model-grid
Closed

almilder wants to merge 3 commits into
mainfrom
fix/pixel-centered-model-grid

Conversation

@almilder

@almilder almilder commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

With points_per_pixel > 1, the model was evaluated on a fine wavelength grid that spanned the first and last detector pixel centres and was then binned in groups of points_per_pixel. Those groups are not centred on the pixels, so the binned model was shifted relative to the data by up to half a pixel.

The fine grid is now extended on each end so that every group is centred on its detector pixel. points_per_pixel: 1 is unchanged.

This is a fix, not a breaking change: the shift is at most half a pixel, so refitting gives values very close to an earlier fit.

Changes

  • form_factor.py, generate_spectra.py: the grid is extended by half a pixel minus half a fine step, using points_per_pixel.
  • prepare.py: for angular data, when lam_res_unit does not divide the number of wavelength pixels, the leftover pixels at the end are dropped instead of being kept as a narrower final unit. Every unit then has the same width, which is what the uniform model grid assumes. Example: lam_res_unit: 5 on 1024 pixels gives 204 units instead of 205. This applies to ARTS2D as well, which previously integrated the narrower bin over its true width (Integrate unresolved ARTS2D resonances into detector bins #143).

Response to review

  • Ragged final wavelength bin: addressed by dropping it from the data, so model and data bins agree.
  • Non-square detectors: not addressed. npts is already derived from CCDsize[1] while the reduction uses CCDsize[0], so a non-square detector is not supported by the existing code either; handling it belongs in a separate change.

Tests

  • New test_binned_model_axis_matches_the_detector_pixel_centers (points_per_pixel 1, 2, 5).
  • The existing ragged-bin test in test_irf_area.py is updated to expect the dropped unit.
  • test_arts1d_forward_pass compares against ThryE-arts1v.npy, generated on the old grid at points_per_pixel: 2. The test shrinks its wavelength range so the new grid lands on the old points, with a note to remove that when the reference is regenerated.
  • Local run (Windows, CPU): forward, angular-objective, recovery-benchmark, and axis tests: 90 passed, 1 skipped. The GPU-only angular fit tests and the full suite were not run locally; CI covers the latter.

Release

No label: merging cuts a patch release (v0.4.1).

🤖 Generated with Claude Code

With points_per_pixel > 1 the fine grid spanned the pixel centres, so binning
it in groups of points_per_pixel shifted the model by up to half a pixel.
Extend the grid by the half-pixel overhang so each group is centred on its pixel.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@ergodic-reviewer ergodic-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uniform-grid correction needs two detector-coordinate edge cases addressed: the actual spectral reduction factor on non-square detectors, and the retained ragged final ARTS wavelength bin. Inline comments explain both.

Postprocessing: existing forward plots/NetCDF output and fit checkpoint/postprocessing paths remain reachable; no separate output pipeline is needed for this grid change. The coordinate mismatches also affect model/data comparisons in those outputs.

Static review only; I did not execute tests, benchmarks, or postprocessing.

Comment thread tsadar/core/physics/generate_spectra.py Outdated
ud_ang=ud_angle,
calc_gain=calc_gain,
n_beta=n_beta,
points_per_pixel=int(config["other"].get("points_per_pixel", 1)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Pass the actual spectral reduction factor to FormFactor

For temporal/imaging spectra, _irfs_from_config() in thomson_diagnostic.py sets n_spectral_pixels = CCDsize[0], and add_electron_IRF() / add_ion_IRF() reduce the fine grid with reshape(irf.n_spectral_pixels, -1). However, prepare_data() derives npts from CCDsize[1] * points_per_pixel, so on a non-square detector the configured points_per_pixel is not necessarily the reduction factor used by the IRF. In the accepted case npts == CCDsize[0] with points_per_pixel > 1, no spectral reduction occurs: the old grid already lands on the calibrated centers, but this new argument causes FormFactor to extend both endpoints anyway. This introduces a wavelength mismatch for both fitted channels. Derive the factor from the actual target spectral pixel count (and apply it to the ion constructor too), with a non-square detector regression alongside the square-detector axis test.

Comment thread tsadar/data/prepare.py Outdated
Comment on lines 223 to 224
config["other"]["lamrangE"] = [axisyE_centers[0], lamrangE_upper]
config["other"]["lamrangI"] = [axisyI[0], axisyI[-1]]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve the ragged final bin's calibrated support

When lam_res_unit does not divide the raw wavelength pixel count, this extrapolates the upper model center as if the final group were full-sized, while data_res_unit and all_axes["epw_y"] still retain the shorter measured group. For fe.dim == 1 (or disabled resonance quadrature), FormFactor uses this uniform range and reduce_ATS_to_resunit() averages the same lam_step in every wavelength bin; nothing restores the last bin's actual center or width. Thus the last model value represents a different wavelength interval from the data it is compared with. This also changes the final center with points_per_pixel == 1, where it previously matched the last calibrated center. The existing test_prepare_data_retains_exact_arts_spectral_edges already supplies a ragged-bin fixture. Keep the regular spacing for full groups, but use the stored grouped_detector_edges support to handle the final model group (or consistently exclude that group from both model and data), and extend that fixture through the 1D-EDF diagnostic path.

…nal ARTS wavelength unit

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@ergodic-reviewer ergodic-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No remaining material findings. The previous review's concerns are addressed: temporal/imaging grids use the actual spectral reduction factor, and angular preparation consistently excludes the ragged wavelength group from data, background, centers, and detector edges. Focused regressions cover these changes.

Postprocessing: the existing forward plots/NetCDF output and fit plots, diagnostics, checkpoint persistence, and standalone postprocessor remain reachable. The grid correction and consistently cropped angular arrays do not require a separate output pipeline.

Validation: static review only; I did not execute tests, benchmarks, or postprocessing. CPU CI is still in progress and GPU CI is skipped. End-to-end angular-fit validation remains a coverage gap, not evidence of a defect.

…are handling

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@ergodic-reviewer ergodic-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One blocking wavelength-coordinate regression remains: the configured oversampling factor is not always the spectrometer's actual reduction factor, including non-square configurations that previously had aligned model/data grids. The inline comment identifies the accepted case and a scoped fix. The ragged angular wavelength group is now consistently excluded from data, background, centers, and detector edges.

Postprocessing: existing forward/fit plots, NetCDF/CSV outputs, angular objective diagnostics, checkpoint persistence, and standalone postprocessing remain reachable. Cropped angular arrays use the matching cropped wavelength axis; no separate output pipeline is needed.

Validation: static review only; I did not execute tests, benchmarks, or postprocessing. CPU CI is in progress and GPU CI is skipped.

ud_ang=ud_angle,
calc_gain=calc_gain,
n_beta=n_beta,
points_per_pixel=config["other"].get("points_per_pixel", 1),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Base the padding on the actual spectral reduction factor

The existing non-square limitation does not make every non-square configuration invalid: _irfs_from_config() accepts any npts divisible by CCDsize[0], and add_electron_IRF() / add_ion_IRF() reduce with reshape(irf.n_spectral_pixels, -1). In particular, when CCDsize[1] * points_per_pixel == CCDsize[0] with points_per_pixel > 1, the old grid already has one sample at each calibrated wavelength center. Passing the configured factor here now extends both endpoints even though the IRF performs no wavelength downsampling. The model therefore stops matching the data's wavelength coordinates, introducing a regression in a previously aligned configuration; fit plots also label these model values with the unchanged calibrated axis.

Please derive the padding factor from the same target spectral-pixel count used by the IRF for temporal/imaging spectra, apply it to both FormFactor constructors, and cover this accepted non-square case in the axis regression. This does not require generalizing the rest of the non-square detector pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant