Repository navigation
Conversation
RectBivariateSpline was constructed with x = arange(NAXIS1) and y = arange(NAXIS2) against data of shape (NAXIS2, NAXIS1), which raises ValueError for any non-square image. It went unnoticed because current detector images are square and the caller in SpectralTrace.rectify compensates by passing (j, i) swapped. Build the spline with axes matching the data layout instead; the (j, i) call convention used by rectify and existing tests is now correct by construction and documented. Results on square images are unchanged. Adds a non-square regression test.
For detailed explanation see AstarVienna#1002, this is basically the same but for `TERCurve`. The new `.__call__()` in this case is exclusively for cubes, because 1D spectra work differently. But those could be simplified as well, and as a result the `combine_two_spectra` function was no longer needed. Also finally remove the long-deprecated `.fov_grid()` methods that were still here. Everything passes without them. Well, except of course `test_current_filter_has_fov_grid_method`...
See AstarVienna#1002 for details and explanation. - Add `Detector.data` property setter to eleminate external access of `Detector._hdu` attribute. - Turn various effect attributes which need currsys/meta resolving into properties. This allows to encapsulate the resolving and make calls to the attributes much cleaner. **If we ever make effects dataclasses with explicit attribute fields rather than kwargs, this will make that process much smoother, because accessing a dataclass field looks identical to accessing a property, we just remove the property definitions then**. - Anything that the `.__call__()` needs, which is taken from the `obj` (in this case usually `Detector`), is resolved before and passed to `.__call__()` as an argument. That means the latter doesn't need to know about `obj`, which is cleaner and matches the PSF case. - I think there are now a few new `np.random.default_rng` cases. This was taken from a branch I started several months ago and forgot about in the much more recent discussion about those. Might not be final. This isn't completely exhaustive for all the effects here, but I tried to cover the "standard" ones...
Somehow forgotten in the previous commit...
overlay_image computed the array origin at which to place small_im as ceil(coords) - small_im.shape // 2. That is only correct when coords sits on the pixel lattice small_im's shape implies: half-integer for even shapes, integer for odd ones. For anything in between it is off by up to a whole pixel, and the direction of the error flips with the parity of the shape. Chunked FOVs hit this whenever the detector footprint is not a whole number of pixels across, as MICADO's is not (189.32 mm / 0.015 mm = 12621.33 px in y). The last chunk of each axis is then a fraction of a chunk wide, so it has a different size and a pixel lattice offset from its neighbours', and it got displaced the opposite way from them. The seam between the two picked up a duplicated or a dropped row/column and the image content shifted by a pixel across it. Snap the origin instead, which is parity-independent. Half-integer origins - a genuine sub-pixel shift, which sub_pixel=True would have to handle properly - round towards the lower index, matching what the old expression did for even shapes.
extract_area_from_imagehdu slices the source array on the source's own pixel grid, growing the slice outwards to whole pixels with floor/ceil so it covers the FOV footprint. It then built the accompanying WCS with create_wcs_from_points([xy0s, xy1s]) - the world corners from *before* that growing, rounded independently. The two disagreed in both size and phase. NAXISn could be a pixel smaller than data.shape, which fits.ImageHDU silently corrects on construction, leaving CRPIXn describing an array of the wrong size; and the WCS put the cutout on the FOV's pixel lattice rather than the source's, which for chunked FOVs is offset from it by a fraction of a pixel. Projecting such a cutout mis-registered it by up to a whole pixel, by differing amounts for neighbouring FOVs, so the checkerboard phase flipped across the chunk seams and a row/column was duplicated or dropped there. The FIXME this replaces described the size half of the problem. Build the cutout WCS by shifting the input WCS's CRPIX by the slice origin instead. The cutout's pixels are input pixels, so this is exact: the cutout keeps the input's pixel lattice and NAXISn always agrees with data.shape. The added integration test reassembles a checkerboard from chunked FOVs with chunk_size=8, over four detector footprints including two that are not a whole number of pixels across. All four fail without this commit and the one before it.
This is the last of the three off-by-one causes and the most invasive of
them: it changes CRVALn for any region that is not a whole number of pixels
across. It is separable from the two commits before it - drop it and the
dropped row and the phase flips stay fixed, but MICADO keeps one duplicated
row at the last chunk boundary in y.
create_wcs_from_points rounds the requested extent to a whole NAXISn and
then centres the grid on the region, folding that region's own rounding
remainder into CRVAL. Each region therefore gets its own sub-pixel lattice.
Chunked FOVs are all carved from one origin in whole-pixel steps - the image
plane, the full-size chunks and the smaller last chunk all measure from the
same detector edge - so they ought to share a lattice, and instead ended up
offset from one another by fractions of a pixel. The projection then has to
snap them to whole pixels, and the last chunk could land a whole pixel off
its neighbours.
MICADO's detector plane is 189.32 mm / 0.015 mm = 12621.33 px tall, so it
hits this on every run: with chunk_size 2048 the last chunk in y is 333.33
px and its lattice is offset from the six full chunks'.
When NAXISn truncates the region, anchor the lower pixel edge at points.min
and let the leftover fraction fall off the far edge; regions carved from the
same origin then coincide. When NAXISn covers the region there is nothing to
truncate, so keep centring - that is what a region smaller than one pixel
needs, where NAXISn was clamped up to 1.
Verified on the full MICADO array (12621 x 12888 image plane, 4 mas
checkerboard, chunk_size 2048), checking that the board's row and column
phase alternates strictly across the whole image plane:
origin/main rows: 10407 bad, cols: 10408 bad,
plus a dropped row at y = 12288
first two commits rows: 166 bad, cols: 0 bad
with this commit rows: 0 bad, cols: 0 bad
FOVManager._get_splits used np.arange(v_min, v_max, step) for the chunk
boundaries. That returns ceil((v_max - v_min) / step) elements, which is one
too many whenever the division rounds just above an integer - as it does for
a span that is an exact multiple of step:
(1.2000000000000002 - -1.2000000000000002) / 0.8 == 3.0000000000000004
Whether the surplus element lands exactly on v_max or one ulp below it
depends on how the platform rounds v_min + i * step. It lands on v_max on
x86, where FovVolumeList.split then discards it (v_max <= value). On arm64
macOS it lands below v_max, which is a perfectly legal split point, so split
accepts it and carves off a volume ~1e-15 px wide. create_wcs_from_points
rounds that up to a whole pixel, so it becomes a spurious one-pixel FOV,
off the pixel lattice, adding flux at the edge of the array.
A detector footprint that is an exact multiple of chunk_size therefore got
4 extra degenerate FOVs on macOS and none on Linux or Windows. Emulating
the arm64 rounding locally, a 24 px footprint at chunk_size 8 gives:
np.arange, x86 rounding 9 volumes, none degenerate
np.arange, arm64 rounding 16 volumes, 4 of width 0
chunk_edges 9 volumes, none degenerate
Compute the edges from the chunk count instead and return only interior
points, so no edge can coincide with either bound on any platform. step
still need not divide the span or be an integer, so this is not a range().
The two test helpers added in the previous commits duplicated the arange
expression and so inherited the same fragility - that is what failed CI on
macos-latest, on exactly the exact-multiple parametrisations. They now call
chunk_edges rather than reimplementing it.
Behaviour on x86 is unchanged: the full MICADO image plane (12621 x 12888)
comes out bit-identical to the previous commit.
This was only used in two places, and one of them has now been refactored to use something else, so no point in keeping the separate function, which in the end was just a one-liner anyway.
It was only used there, and the module is small enough that keeping it elsewhere doesn't make much sense. Also removes one cross-import between optics and effects. There is still `get_filter`, but maybe we can remove that as well, and the whole scale function actually, if we used the existing functionality in SpeXtra / synphot directly... The functions `zero_mag_flux` and `get_zero_mag_spectrum` were not used anywhere except in their own unit tests, and are definetely redundant with SpeXtra / synphot, so were removed entirely.
- Remove two local vars only needed once. - Consistent naming of detector manager instance and list.
Bumps the main-dependencies group with 1 update: [tqdm](https://github.com/tqdm/tqdm). Updates `tqdm` from 4.70.0 to 4.70.1 - [Release notes](https://github.com/tqdm/tqdm/releases) - [Commits](tqdm/tqdm@v4.70.0...v4.70.1) --- updated-dependencies: - dependency-name: tqdm dependency-version: 4.70.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: main-dependencies ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps the dev-dependencies group with 1 update: [sphinxcontrib-mermaid](https://github.com/mgaitan/sphinxcontrib-mermaid). Updates `sphinxcontrib-mermaid` from 2.1.0 to 2.1.1 - [Changelog](https://github.com/mgaitan/sphinxcontrib-mermaid/blob/master/CHANGELOG.md) - [Commits](mgaitan/sphinxcontrib-mermaid@2.1.0...2.1.1) --- updated-dependencies: - dependency-name: sphinxcontrib-mermaid dependency-version: 2.1.1 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: dev-dependencies ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [urllib3](https://github.com/urllib3/urllib3) from 2.7.0 to 2.8.0. - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst) - [Commits](urllib3/urllib3@2.7.0...2.8.0) --- updated-dependencies: - dependency-name: urllib3 dependency-version: 2.8.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [tornado](https://github.com/tornadoweb/tornado) from 6.5.8 to 6.5.9. - [Changelog](https://github.com/tornadoweb/tornado/blob/master/docs/releases.rst) - [Commits](tornadoweb/tornado@v6.5.8...v6.5.9) --- updated-dependencies: - dependency-name: tornado dependency-version: 6.5.9 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [notebook](https://github.com/jupyter/notebook) from 7.6.1 to 7.6.3. - [Release notes](https://github.com/jupyter/notebook/releases) - [Changelog](https://github.com/jupyter/notebook/blob/@jupyter-notebook/tree@7.6.3/CHANGELOG.md) - [Commits](https://github.com/jupyter/notebook/compare/@jupyter-notebook/tree@7.6.1...@jupyter-notebook/tree@7.6.3) --- updated-dependencies: - dependency-name: notebook dependency-version: 7.6.3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [jupyterlab](https://github.com/jupyterlab/jupyterlab) from 4.6.3 to 4.6.4. - [Release notes](https://github.com/jupyterlab/jupyterlab/releases) - [Changelog](https://github.com/jupyterlab/jupyterlab/blob/main/RELEASE.md) - [Commits](https://github.com/jupyterlab/jupyterlab/compare/@jupyterlab/lsp@4.6.3...@jupyterlab/lsp@4.6.4) --- updated-dependencies: - dependency-name: jupyterlab dependency-version: 4.6.4 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge the already-adopted upstream ScopeSim baseline through 2d24516. Retain the fork effects and reconcile upstream detector-manager, coordinate and surface-table changes before the new spectroscopic work.
Part 02/29 of the Caltech review series. Review the diff against
main; the final destination ismain.Review focus: scopesim/optics/optical_train.py; scopesim/optics/surface.py; upstream changes.
Validation: 100 targeted OpticalTrain/FOV-manager/TERCurve/PRNU checks passed. Shared downstream boundary checks also pass.
Source commits/snapshots:
2d245168,e3e9b62b.Use a merge commit to preserve the stack ancestry and pinned commits. After this merges, retarget the next PR in this repository to its default branch. Do not squash/rebase an earlier stack entry without rebuilding its descendants and dependency pins.