Skip to content

Remove the spread model from ShapePipe - #911

Merged
cailmdaley merged 7 commits into
developfrom
fix/makecat
Sep 28, 2026
Merged

Remove the spread model from ShapePipe#911
cailmdaley merged 7 commits into
developfrom
fix/makecat

Conversation

@cailmdaley

@cailmdaley cailmdaley commented Sep 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Martin's call: the spread model didn't really work when he last ran it, and it separates stars from galaxies worse than the downstream classification (sp_validation's size-based selection), which is what we use. So this PR removes it from ShapePipe rather than fixing it.

Removed

  • spread_model_runner and spread_model_package.
  • make_cat's spread-model path: save_sm_data, the optional fourth sexcat_sm input, and the SM_DO_CLASSIFICATION / SM_STAR_THRESH / SM_GAL_THRESH keys. make_cat_runner now takes exactly three inputs: tile sexcat, galaxy PSF and ngmix.
  • SM_DO_CLASSIFICATION = False from workflow/config/cfis/config_tile_Mc.ini and example/cfis_image_sims/config_tile_Mc_psfex.ini. The SPREAD_CLASS / SPREAD_MODEL / SPREADERR_MODEL entries are gone from final_cat.param, where they were already commented out, and from example/unions_800/cat_matched.param. The docs and refs.bib no longer mention the spread model.

Consumers: the committed chain already ran with three inputs and classification off, so its output catalogue is unchanged. sp_validation develop doesn't read any SPREAD_* column; its column-migration doc already records their removal. Its last leftovers (a dead do_spread_model flag and a docstring) are removed in CosmoStat/sp_validation#364. cs_util has no references.

Decision record: in astra.yaml, catalogue_assembly.star_galaxy_classification keeps deferred_downstream. It is now [HARDCODED] (there is no config key anymore), and the spread_model_inline option is excluded for the two reasons above. universes/committed.yaml doesn't change. astra-tools validate passes.

Tests: the spread-model path tests are replaced by one make_cat_runner end-to-end test, which checks that every detection is kept and that no classification column is written. Full suite: 686 passed, 1 skipped. The 2 errors are the known ones in tests/cluster/test_star_shear_response.py.

The legacy CANFAR scripts (scripts/sh/job_sp_canfar.bash, init_run_exclusive_canfar.sh) and summary_params_pre_v2.py are untouched. They drive the old v1 run layout (run_sp_tile_PsViSmVi, --sm) through configs that are no longer in the repo.

Closes #910

— Claude (Opus) on behalf of Cail

🤖 Generated with Claude Code

cailmdaley and others added 4 commits September 26, 2026 03:50
@cailmdaley @claude
fix(make_cat): return actual saved count from save_sm_data
8e91bdd 
save_sm_data returned the n_obj argument it was given instead of the
number of rows it actually saved, so the runner's SExtractor-vs-
spread-model size check compared a value with itself and could never
detect a mismatch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014bvNTrAmZxcfb1ee83ApPK
@cailmdaley @claude
fix(make_cat): fix crashing size-mismatch log and honest spread-model…
81ed2d0 
… logging

- The size-mismatch warning called w_log(...) directly; w_log is a
  logger, not a callable, so a real mismatch crashed instead of being
  reported. Use w_log.warning(...), matching other runners, and fix
  the malformed message.
- With no spread-model input, the runner logged "setting spread model
  to 99" but wrote no SPREAD_MODEL column at all. The log now says
  what actually happens; behaviour for the committed three-input path
  (SM_DO_CLASSIFICATION off) is unchanged.
- SM_DO_CLASSIFICATION = True with no spread-model input is now a
  config error (there is nothing to classify on), raised clearly
  instead of being silently ignored.
- A missing SM_STAR_THRESH/SM_GAL_THRESH under classification now
  raises a clear ValueError instead of a bare configparser.NoOptionError.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014bvNTrAmZxcfb1ee83ApPK
@cailmdaley @claude
test(make_cat): cover spread-model path defects in save_sm_data/make_…
e22bad1 
…cat_runner

One test per defect fixed in make_cat_runner/save_sm_data, each
verified to fail against the pre-fix code and pass after: save_sm_data
returning its true saved count, the size-mismatch warning logging
without crashing, honest no-input logging with no SPREAD_MODEL column
written (matching the committed three-input path), refusing
SM_DO_CLASSIFICATION without a spread-model input, and a clear error
for a missing SM_STAR_THRESH/SM_GAL_THRESH.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014bvNTrAmZxcfb1ee83ApPK
@cailmdaley @claude
test(make_cat): fix fixture defects flagged in review
e26b2f6 
- The classification-without-input test never wrote its ngmix
  catalogue, so on the pre-fix code it failed with
  catalogueFileNotFound instead of exercising the intended assertion.
  It now writes a matching ngmix catalogue and fails with "DID NOT
  RAISE ValueError" against the pre-fix runner, confirmed locally.
- Single-field structured arrays (NUMBER only) get collapsed by
  FITSCatalogue._save_to_fits into one row with a vector-valued
  column whenever save_sextractor_data re-saves them, so every
  SExtractor-like fixture now carries a second scalar field
  (_numbered_data) and tests assert the NUMBER column comes back as
  scalar ids matching the input.
- Present tense throughout: docstrings describe what each test
  protects, not the prior buggy behaviour; the section heading drops
  the branch-name reference.
- New tests grouped under TestSpreadModelPathDefects so a concurrent
  append to this file's end (PR #854) merges as one class-sized block.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014bvNTrAmZxcfb1ee83ApPK
@cailmdaley
cailmdaley marked this pull request as ready for review September 26, 2026 12:20
@cailmdaley

cailmdaley commented Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

this PR seems straightforward and mergeable, but @martinkilbinger should should spread-model classification really be off? should we flip SM_DO_CLASSIFICATION while we're at it?

@cailmdaley cailmdaley mentioned this pull request Sep 28, 2026
Merged
@cailmdaley

cailmdaley commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Martin says spread model classification is unnecessary and can be totally removed.

cailmdaley and others added 2 commits September 28, 2026 16:57
@cailmdaley @claude
Merge origin/develop into fix/makecat
3786fee 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cailmdaley @claude
Remove the spread model from ShapePipe
6c2fc6f 
The spread-model star/galaxy classifier did not work reliably when last
run and separates stars from galaxies worse than the downstream
(sp_validation, size-based) classification, so ShapePipe no longer
computes or carries it.

- Delete spread_model_runner and spread_model_package.
- make_cat_runner takes exactly three inputs (tile sexcat, galaxy PSF,
  ngmix); save_sm_data and the SM_DO_CLASSIFICATION / SM_STAR_THRESH /
  SM_GAL_THRESH keys are gone, along with the tests of that path.
- Drop SM_DO_CLASSIFICATION from the committed and image-sims make_cat
  configs and the SPREAD_* columns from final_cat.param and
  example/unions_800/cat_matched.param; tidy docs and refs.bib.
- astra.yaml: star_galaxy_classification is now [HARDCODED]; the
  spread_model_inline option is excluded, with the reasons.
- A make_cat_runner end-to-end test checks every detection is kept with
  no classification column.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cailmdaley cailmdaley changed the title make_cat: correct spread-model path defects in catalogue assembly Remove the spread model from ShapePipe Sep 28, 2026
@cailmdaley @claude
Merge origin/develop into fix/makecat
af7499d 
Resolve tests/module/test_make_cat.py by keeping both the make_cat_runner
end-to-end test (no spread-model column) and #854's metacal-flag tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uemfjv9ybCwtKtprksZbVY
@cailmdaley cailmdaley mentioned this pull request Sep 28, 2026
Merged
@cailmdaley

cailmdaley commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

LGTM

@cailmdaley
cailmdaley merged commit 791c90d into develop Sep 28, 2026
3 checks passed
@cailmdaley
cailmdaley deleted the fix/makecat branch September 28, 2026 16:35
cailmdaley added a commit that referenced this pull request Sep 28, 2026
@cailmdaley @claude
Merge origin/develop (#879 squash, #875, #854, #905, #911, #894) into…
a1cb926 
… feat/wire-external-masks

Resolved by keeping develop's side everywhere #879's pre-squash content
conflicted, then re-applying this branch's own changes (a846e08..dc11dab):

- astra.yaml: the mask-cut decision moves into the masking sub-analysis as
  masking.mask_default_cut, in the #875 format (no Anchor sentence; the
  make_cat contract tag cites it). masking.sky_mask_application now selects
  catalogue_columns, since the committed config sets MASK_EXT_PATHS;
  config_tile_Mc.ini's MASK_EXT_PATHS carries its tag. universes pinned.
- cfis_image_sims/config_tile_Mc.ini: rebuilt from develop's cfis copy
  (spread model gone), still without MASK_EXT_PATHS or per-epoch slots.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cailmdaley added a commit that referenced this pull request Sep 28, 2026
@cailmdaley @claude
Merge origin/develop (#879 squash, #875, #854, #905, #911, #894) into…
8ec3662 
… feat/retire-versioning

Resolved by keeping develop's side where #879's pre-squash content
conflicted, then re-applying this branch's own changes (a846e08..bfb7efc).
Two by hand: docs/source/pipeline_canfar.md stays retired (develop's one-line
path fix goes with it), and the rebuilt tutorial drops the spread-model step
that #911 removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cailmdaley added a commit that referenced this pull request Sep 28, 2026
@cailmdaley @claude
Merge origin/develop into feat/unions-catalogue-tile-detection
0a0396a 
Brings in #879 (campaign merges), #911 (spread model removed), #854,
#905 and #923.

- run_config: input_types:/machines: table resolver kept; develop's
  required `run:` and unresolved-$ check folded in (now over every
  non-table key).
- create_final_cat.read_data keeps develop's raise on a missing column;
  the branch's skip-missing path is dropped (final_cat_merge is the one
  producer of final_cat_<run>.hdf5).
- final_cat.param carries both NUMBER and TILE_UNIQUE_ID.
- DAG tests follow the committed tile_detection default (data plans
  tile_get_catalogue); params pin regenerated for the tile_detect switch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DksuyF9YrZAwHQeAMXsxp4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

make_cat: spread-model size check can never fire; SM_DO_CLASSIFICATION silently ignored without its input

1 participant

@cailmdaley