PhotoPipe + ShapePipe - #310
Conversation
Pushed by the lint gate.
|
✅ ruff is clean — nothing to fix here. |
cailmdaley
left a comment
cailmdaley
left a comment
There was a problem hiding this comment.
Thanks Martin — the fill script looks solid (resumable, chunked, good failure accounting). Two things below: cat_config.yaml picked up merge damage and currently doesn't parse, and one question about row alignment. Once the config is fixed I'm happy to approve.
— Claude (Fable) on behalf of Cail; review discussed and approved by Cail.
| path: unions_shapepipe_psf_2024_v1.4.a.fits | ||
| hdu: 1 | ||
| patch_number: 100 | ||
| >>>>>>> upstream/develop |
There was a problem hiding this comment.
This looks like leftover merge conflict resolution — the file no longer parses as YAML (yaml.safe_load fails on this line). Two more traces of the same merge: the SP_v1.4.6.3_uncal{,_w_iv,_w_1} entries appear twice (around lines 911 and 1181), and SP_v1.4.8's star: block lost its path: line. Fixing this file is the one blocker.
| n_hdf5 = len(hdf5_indices) | ||
| n_fits = len(fits_data) | ||
|
|
||
| if n_hdf5 != n_fits: |
There was a problem hiding this comment.
Question to make sure I trust the output: within a tile, is the row order of the PhotoPipe FITS guaranteed to match the row order of the comprehensive HDF5? The write assumes one-to-one alignment in order and only the row count is checked — if PhotoPipe ever re-sorts objects within a tile, photo-z's would be silently assigned to the wrong galaxies. If order is guaranteed by construction, all good; otherwise a per-tile spot check (e.g. compare RA of the first/last few rows and skip the tile on mismatch, like the size check) would make that failure impossible.
There was a problem hiding this comment.
In principle the order is the same, but I agree that a check would be good. I had a ra and dec distance check implemented, but this was extremely slow. Doing the check on just a few rows could work well. Can you implement @claude ?
Conflicts: check.txt, format.txt, report.md — ruff scratch output accidentally committed on photoz and deliberately untracked on develop (fdd78f0 "Stop tracking CI lint-gate scratch files"). Resolved by taking develop's deletion; the content was stale lint output already fixed by 2d5ef07. Also repair two leftovers from the earlier develop->photoz merge in cosmo_val/cat_config.yaml: - drop the duplicated, 3-space-indented SP_v1.4.6.3_uncal{,_w_iv,_w_1} block that had been spliced into the middle of SP_v1.4.8's star section (the entries exist once, correctly, further down); - restore SP_v1.4.8 star: path:, which that splice had swallowed.
The fill pairs FITS row k with the k-th HDF5 row of the tile (sorted-index order) and only ever verified the row *count*, so a PhotoPipe tile ordered differently from the comprehensive catalogue would be filled with silently mismatched photo-z. After the size check, compare RA/Dec for a small sample of rows -- up to five at each end plus evenly spaced interior rows, --n_check_rows (default 10), --check_tol_arcsec (default 0.5). Only the sampled HDF5 rows are read (one fancy-index into the already-sorted index array), so the cost is a handful of point reads per tile rather than a full per-row match. A failing tile is warned about, counted as a row-order mismatch in the end-of-run summary, counted towards the consecutive-failure abort, and skipped without being added to done_tiles -- exactly as a size mismatch is, so a resume retries it. --n_check_rows 0 disables the check. HDF5 columns are RA/Dec (cat_config.yaml ra_col/dec_col for SP_v1.4.x). The FITS names are resolved at runtime from a candidate list; ALPHA_J2000 / DELTA_J2000 is first, verified against a real DR6 tile (/n17data/UNIONS/WL/photometry/UNIONS_DR6/UNIONS.001.227_SP_ugriz_photoz_ext.cat). If no candidate pair matches, the check disables itself with a warning rather than skipping tiles. Test: synthetic 3-tile HDF5 + FITS pair, tiles interleaved so the non-contiguous write path runs too; the two aligned tiles fill, the tile with reversed FITS rows is skipped and left empty, and --n_check_rows 0 reproduces the old unchecked behaviour.
…f none A sampled row with a non-finite or sentinel (|Dec| > 90) coordinate on either side carries no information about the FITS/HDF5 pairing. Before, such a row made `sep <= tol` False (a false row-order failure), and an all-NaN sample raised a RuntimeWarning from np.nanmax. Now those rows are excluded from the comparison, n_checked reports only the rows actually compared, and a tile whose sample has no comparable row is treated as unverifiable and skipped (distinct warning), so it is retried on resume rather than written blind. Adds a direct unit test covering partial/all-invalid samples, a reversed remainder, and a single-row tile (one-element h5py fancy index). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cailmdaley
left a comment
cailmdaley
left a comment
There was a problem hiding this comment.
Done: the fill now spot-checks RA/Dec on ~10 rows per tile (both ends plus interior, 0.5" tolerance, --n_check_rows 0 disables) and skips a tile on mismatch so a resume retries it. Also merged develop and fixed cat_config.yaml: the duplicated SP_v1.4.6.3_uncal* block had been spliced into SP_v1.4.8's star section. Approving.
— Claude on behalf of Cail
2 participants
PR adds functionality to merge PhotoPipe outputs with ShapePipe catalogues, and to create joint comprehensive SP + PP catalogues.