Repository navigation
Conversation
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>
There was a problem hiding this comment.
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.
| ud_ang=ud_angle, | ||
| calc_gain=calc_gain, | ||
| n_beta=n_beta, | ||
| points_per_pixel=int(config["other"].get("points_per_pixel", 1)), |
There was a problem hiding this comment.
[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.
| config["other"]["lamrangE"] = [axisyE_centers[0], lamrangE_upper] | ||
| config["other"]["lamrangI"] = [axisyI[0], axisyI[-1]] |
There was a problem hiding this comment.
[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>
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
[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.
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 ofpoints_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: 1is 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, usingpoints_per_pixel.prepare.py: for angular data, whenlam_res_unitdoes 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: 5on 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
nptsis already derived fromCCDsize[1]while the reduction usesCCDsize[0], so a non-square detector is not supported by the existing code either; handling it belongs in a separate change.Tests
test_binned_model_axis_matches_the_detector_pixel_centers(points_per_pixel1, 2, 5).test_irf_area.pyis updated to expect the dropped unit.test_arts1d_forward_passcompares againstThryE-arts1v.npy, generated on the old grid atpoints_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.Release
No label: merging cuts a patch release (
v0.4.1).🤖 Generated with Claude Code