Skip to content

Coverage mask#699

Open
martinkilbinger wants to merge 17 commits intoCosmoStat:developfrom
martinkilbinger:coverage
Open

Coverage mask#699
martinkilbinger wants to merge 17 commits intoCosmoStat:developfrom
martinkilbinger:coverage

Conversation

@martinkilbinger
Copy link
Copy Markdown
Contributor

@martinkilbinger martinkilbinger commented Jan 14, 2026

Summary

Added library files and classes to create healsparse coverage mask files.
The following steps can be performed:

  • Download exposure FITS headers from VOSpace
  • Extract CCD corner coordinates
  • Create hsp coverage map

Wait for #702 .

Closes #700 .

Reviewer Checklist

Reviewers should tick the following boxes before approving and merging the PR.

  • 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
  • All of the reviewer's comments have been satisfactorily addressed by the developer

@martinkilbinger martinkilbinger self-assigned this Jan 16, 2026
@martinkilbinger martinkilbinger marked this pull request as ready for review January 16, 2026 07:18
@cailmdaley
Copy link
Copy Markdown
Contributor

Merged latest develop into this branch (no force-push — just a merge commit). This absorbs #702, shrinking the effective PR diff from 43 files / ~29.6k lines to 11 files / +2036 / -165, which makes the genuinely new coverage-mask code reviewable.

Conflicts resolved:

  • pyproject.toml — kept both sides: fitsio extra from develop + plot in dev from this branch
  • docs/source/pipeline_canfar.md — kept develop's new "Create matched star catalogue" section; kept this branch's richer "Create coverage mask" docs (including the -d headers_v1.3 symlink-reuse notes and full build_coverage_map command)

Nothing structural was touched. Full review to follow.

@cailmdaley
Copy link
Copy Markdown
Contributor

Review of coverage-mask work

Good overall — the division of labor between this PR (builds coverage from exposure polygons) and sp_validation (consumes/applies/validates masks) is clean, and using cs_util.plots.FootprintPlotter for plotting is the right factoring.

Note on scope: after merging develop into this branch, the effective diff is 11 files / +2036 / −165 — concentrated on the 8 genuinely new files.

Blocking

  • coverage_run.py:97–139run_pipeline can't work as written. All three stages (run_download_headers, run_extract_corners, run_build_coverage) are called with the same args, but each uses -i/-o/etc. with different meanings. The coverage_pipeline entry point in pyproject.toml therefore runs only the first stage correctly. Either prefix options per stage, or drive the pipeline from a config file.
  • coverage_map_builder.py:19 vs :321 contradict. Module-level from shapepipe.utilities.coverage_plotter import CoveragePlotter runs at import time — if the plot stack is missing, the whole module fails before reaching the except ImportError at L321. That try/except is unreachable. Either move the import into the try block, or drop the try entirely.
  • coverage_map_builder.py:323 — wrong dependency in error message. Says "Install sp_validation package for plotting support". Should say cs_utilCoveragePlotter's hard dep is cs_util.plots.FootprintPlotter, not sp_validation.
  • field_corners_extractor.py:135expnum = int(path[end - 6 : end]) hardcodes 6-digit exposure numbers. Silently corrupts if an exposure number ever exceeds 999999. Parse by the - separator in the CCD filename, or use a regex.

Non-blocking but worth fixing

  • field_corners_extractor.py:149–152 — the hardcoded MegaCam HDU indices (0, 8, 35, 27) and pixel coords (2079, 0, 32) work but are magic. At minimum a short comment naming the MegaCam corner convention.
  • field_corners_extractor.py has two copies of the same WCS-parsing logic — process_single_header (static, L113–162, exists only so multiprocessing can pickle it) and get_wcs_from_header (L164–195). Extract to a module-level helper.
  • coverage_plotter.py:237FootprintPlotter._regions[region] reaches into a single-underscore private attribute. Fragile. Either cs_util should expose a public accessor for region lookup, or duplicate the dict locally.
  • coverage_map_builder.py:175new_hsp = hsp_map + 0 is an opaque copy idiom. Use .copy() if healsparse has it, otherwise a one-line comment explaining why +0.
  • coverage_map_builder.py:224verbose = self._params["verbose"] but verbose is never declared in params_default. Relies on cs_util.args.parse_options injecting it. Declare it explicitly in params_default so the contract is local.
  • header_downloader.py:36vospace_path = "vos:cfis/pitcairn" is a fine default but CFIS-specific. Worth a prominent note in help string or docs.
  • build_and_plot_coverage_maps.shBUILD_NSIDE=131072 (~0.1″ resolution). The Python class default in coverage_map_builder.py:39 is nside=2048. The docs say 131072 matches the bit-mask resolution — could you confirm that's the intended pairing, and maybe note it in a comment at the top of the bash script?

Tests

No tests for any of the new modules. Same gap as #702. A small test at least for CoverageMapBuilder.median_filter and FieldCornersExtractor.process_single_header would catch a lot. Not a blocker for merge, but worth tracking.

Side note on FootprintPlotter

Not a concern for this PR (you correctly import from cs_util), but flagging: sp_validation has a stale copy of FootprintPlotter at src/sp_validation/plots.py:402 that has diverged from cs_util's. Opening a separate cleanup PR on sp_validation to delete the local copy and import from cs_util.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To do

Development

Successfully merging this pull request may close these issues.

[NEW FEATURE] Create coverage masks for UNIONS

2 participants