Skip to content

refactor: drop sip_tpv, rely on astropy for TPV WCS (closes #713)#716

Merged
martinkilbinger merged 3 commits intodevelopfrom
fix/drop-sip-tpv
Apr 24, 2026
Merged

refactor: drop sip_tpv, rely on astropy for TPV WCS (closes #713)#716
martinkilbinger merged 3 commits intodevelopfrom
fix/drop-sip-tpv

Conversation

@cailmdaley
Copy link
Copy Markdown
Contributor

Summary

Removes the sip_tpv dependency from shapepipe entirely. Modern astropy (≥5) parses TPV distortion natively via WCSLIB, as do SExtractor, PSFEx, and every other downstream WCS consumer in the pipeline, so the keyword-level pv_to_sip conversion that split_exp used to run was no longer buying us anything.

Dropping the conversion lets us drop sip_tpv itself — an unmaintained 2017 package whose v1.1 imports pkg_resources at module load and is therefore incompatible with setuptools>=81. That's the actual root cause of #713 (rewritten to reflect this). PR #714's setuptools<81 pin is a symptom workaround that becomes unnecessary once sip_tpv is gone.

What changed

Code

  • split_exp_package/split_exp.py: drop import sip_tpv as stp and the stp.pv_to_sip(h) call; remove the now-dead transf_coord parameter from process/create_hdus; fix the module docstring.
  • split_exp_runner.py, python_example_runner.py: remove sip_tpv from depends=[…].
  • find_exposures_runner.py: remove sip_tpv from depends=[…] — spurious declaration, find_exposures_package never imported it.

Environment

  • environment.yml, environment-dev.yml, Dockerfile, install_shapepipe: drop sip_tpv.
  • docs/source/dependencies.md: drop the sip_tpv row.
  • docs/source/refs.bib: drop the orphan shupe:12 entry.

Tests (new)

  • src/shapepipe/tests/test_split_exp.py: three unittests that build a synthetic multi-CCD FITS with TPV distortion, run SplitExposures, and verify:
    1. Per-CCD output headers' WCS matches the source exposure's WCS at several pixel samples.
    2. The headers*.npy side-output round-trips pixel → world → pixel.
    3. flag output files are stored as int16.

All four modules in src/shapepipe/tests/ still pass locally (7/7).

Not touched

  • scripts/jupyter/wcs.ipynb still imports sip_tpv — it's an exploratory notebook last edited ~2.5 years ago. Left alone to keep the diff scoped.

Interaction with open PRs

Reviewer Checklist

  • The PR targets the develop branch
  • The PR is assigned to the developer
  • The PR has appropriate labels
  • The PR is included in appropriate projects and/or milestones
  • The PR includes a clear description of the proposed changes
  • If the PR addresses an open issue the description includes "closes #"
  • The code and documentation style match the current standards
  • Documentation has been added/updated consistently with the code
  • All CI tests are passing
  • API docs have been built and checked at least once (if relevant)
  • All changed files have been checked and comments provided to the developer

Closes #713.

The only functional use of sip_tpv in shapepipe was
`split_exp.create_hdus` calling `stp.pv_to_sip(h)` to rewrite each
per-CCD header's TPV distortion keywords as SIP before saving. Modern
astropy (>=5) parses TPV natively via WCSLIB, as do SExtractor, PSFEx,
and every other downstream WCS consumer in the pipeline, so the
keyword-level conversion was no longer buying us anything.

Removing the conversion lets us drop sip_tpv entirely — an unmaintained
2017 package whose v1.1 imports `pkg_resources` at module load and is
incompatible with setuptools>=81 (the underlying cause of the
ModuleNotFoundError in #713; PR #714's setuptools<81 pin is a symptom
workaround).

Changes:
- split_exp: remove sip_tpv import + pv_to_sip call; drop the now-dead
  `transf_coord` plumbing through `process` / `create_hdus`; update
  module docstring accordingly.
- Remove sip_tpv from `depends=[...]` in split_exp_runner,
  python_example_runner, and find_exposures_runner (the last was a
  spurious declaration — find_exposures_package never imported it).
- Drop sip_tpv from environment.yml, environment-dev.yml, Dockerfile,
  install_shapepipe, docs/source/dependencies.md, and the orphan
  shupe:12 BibTeX entry.
- New: `src/shapepipe/tests/test_split_exp.py` — three unittests that
  build a synthetic multi-CCD FITS with TPV distortion, run
  SplitExposures, and verify (1) per-CCD header WCS matches the source
  for several pixel samples, (2) the saved headers .npy round-trips
  pixel→world→pixel, (3) flag output uses int16.

`scripts/jupyter/wcs.ipynb` still imports sip_tpv; it's an exploratory
notebook last touched ~2.5 years ago and left alone.
Adds integration tests that verify the premise of the sip_tpv removal:
that TPV distortion headers survive the split_exp → SExtractor → PSFEx
chain without keyword-level conversion.

- SExtractorTPVTestCase: builds a synthetic 512×512 image with a
  CFIS-scale TPV header (non-trivial radial term), injects 9 Gaussian
  point sources, runs source-extractor, and asserts each detection's
  ALPHA_J2000/DELTA_J2000 agrees with astropy's TPV pix→world mapping
  to within 10 mas (~0.05 px at 0.187"/pix). Observed: max separation
  ~0.5 mas on SExtractor 2.25.0.
- PSFExTPVTestCase: runs the same kind of image through source-extractor
  with FITS_LDAC output, asserts LDAC_IMHEAD preserves the PV keywords
  verbatim, runs psfex on the LDAC, and asserts a PSF model with
  ACCEPTED > 0 and CHI2 < 5. Also re-reads the LDAC-embedded header via
  astropy and asserts the WCS matches the source header to <1e-9 px.

Gated on `shutil.which("source-extractor")` / `which("psfex")`;
skipped on machines without the external binaries.
@cailmdaley
Copy link
Copy Markdown
Contributor Author

Follow-up: empirical verification of the TPV-downstream premise.

The previous state of this PR relied on an inference — "SExtractor and PSFEx link against WCSLIB, which handles TPV, therefore they handle TPV" — without actually running the external tools. Adding integration tests to actually verify it:

test_tpv_external_tools.py (commit f35cec51)

  • SExtractorTPVTestCase — builds a synthetic 512×512 image with a CFIS-scale TPV header (non-trivial radial terms PV{1,2}_{4,6}), injects 9 Gaussian point sources at known pixel positions, runs source-extractor 2.25.0, and asserts each detection's ALPHA_J2000 / DELTA_J2000 agrees with astropy's TPV pix→world mapping.

    • Tolerance: 10 mas (~0.05 px at 0.187"/pix).
    • Observed max separation: 0.5 mas — essentially pure centroiding noise; the WCS math is exact.
  • PSFExTPVTestCase — same kind of image → SExtractor FITS_LDAC → PSFEx. Asserts:

    1. LDAC_IMHEAD preserves every PV keyword verbatim through SExtractor (it does).
    2. psfex 3.21.1 exits successfully on a TPV-headed LDAC (it does; 9 stars loaded, 9 accepted, CHI2 ≈ 0.81).
    3. The LDAC-embedded WCS round-trips pix↔world identically to the source header (1e-9 px agreement).

Both tests are gated on shutil.which("source-extractor") / which("psfex") and skip gracefully otherwise.

Combined with the earlier code audit showing every shapepipe module consumes WCS via astropy.wcs.WCS(header) with zero keyword-level introspection, this closes the "what about downstream?" question for me. Ready for review.

#715 removed treecorr from environment.yml, environment-dev.yml,
Dockerfile, and install_shapepipe, but left the dependency row in
docs/source/dependencies.md and the now-orphan `jarvis:04` BibTeX
entry in docs/source/refs.bib (no other references).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cailmdaley
Copy link
Copy Markdown
Contributor Author

Piggybacked a small follow-up: removed the TreeCorr row from docs/source/dependencies.md and its orphan jarvis:04 BibTeX entry from docs/source/refs.bib. Both were missed in #715's cleanup. Same file diff as the sip_tpv/shupe:12 removal; natural home rather than a new PR for 2 changes.

@martinkilbinger
Copy link
Copy Markdown
Contributor

I ran the two tests and they passed. Not sure how strict these really are, but I guess we can merge this PR. This package seems to be before support in astropy was implemented.

@martinkilbinger martinkilbinger merged commit 6ab2a24 into develop Apr 24, 2026
1 check passed
@martinkilbinger martinkilbinger deleted the fix/drop-sip-tpv branch April 24, 2026 13:12
@cailmdaley cailmdaley mentioned this pull request Apr 24, 2026
12 tasks
cailmdaley added a commit to martinkilbinger/shapepipe-1 that referenced this pull request Apr 24, 2026
Commit 78c2668 deleted read_ext_cat_package/ and read_ext_cat_runner.py
while renaming consumers to read_ext_sexcat_runner, but never added the
renamed module. Likewise the commit moved Vizier retry logic out of
create_star_cat.py and mask.py into shapepipe.utilities.vizier but that
module was never created. This left the imports broken on v2.0:

- mask.py:22 and create_star_cat.py:30 import shapepipe.utilities.vizier
  (ImportError on module load — mask is a core pipeline stage)
- config_tile_Uc.ini, test_tile_det.py, and run_job_sp_canfar_v2.0.bash
  all reference read_ext_sexcat_runner (module-not-found at pipeline run)

Changes:

* shapepipe.utilities.vizier — new module exposing query_vizier(ra, dec,
  radius_arcmin, cat_id), consolidating the retry logic from mask.py and
  create_star_cat.py (server fallback, timeout escalation, random stagger,
  double-precision coord fix inherited from mask.py).

* read_ext_sexcat_package/ + read_ext_sexcat_runner.py — restored from
  the deleted read_ext_cat_package with:
  - Renamed symbols (read_ext_sexcat, make_ldac_from_ascii, etc).
  - Tile-id overflow guard: the encoding tile_id = RRR*1000 + DDD silently
    collided for dec_idx >= 1000; extracted into _tile_id_from_file_number_string
    with an explicit bounds check.

* pyproject.toml: dropped dead deps — treecorr and Stile (removed in CosmoStat#715),
  sip_tpv (removed in CosmoStat#716, merged today). These were stale holdovers on
  v2.0 that never got swept after the corresponding develop cleanups.

* Dockerfile.jupyter: FROM points at ghcr.io/cosmostat/shapepipe:v2.0
  instead of the martinkilbinger fork tag, so upstream builds work.

Addresses the two new blockers flagged in PR CosmoStat#706 re-review:
CosmoStat#706 (comment)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cailmdaley added a commit that referenced this pull request Apr 24, 2026
#716 (merged 2026-04-24) removed sip_tpv from shapepipe, which was the
transitive source of split_exp.py's pkg_resources import chain. On the
post-merge develop container, both split_exp entries now import cleanly:

- shapepipe.modules.split_exp_package.split_exp
- shapepipe.modules.split_exp_runner

Also dropped the matching runner_metadata entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

2 participants