Skip to content

Coverage mask#699

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

Coverage mask#699
martinkilbinger wants to merge 25 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.

@martinkilbinger
Copy link
Copy Markdown
Contributor Author

I fixed some of the problems:

  • coverage_run.py:97–139 — run_pipeline: Fixed, removed this non-functioning pipeline runner.
  • coverage_map_builder.py: Fixed.

And added comments for the hard-coded MegaCAM/UNIONS stuff.

For the rest I think claude can do a better job than me to fix.

martinkilbinger and others added 4 commits April 21, 2026 15:39
- Extract _expnum_from_path, _parse_header_to_wcs, _megacam_field_corners
  as module-level helpers. process_single_header (static, for
  multiprocessing) and the instance methods now share one implementation.
- Remove dead get_wcs_from_header and get_megacam_field (duplicated
  logic that had already been inlined into process_single_header).
- Fix re.search(r'(\d+)\.txt') missing its subject string. The new
  helper also replaces the hard-coded p[end-6:end] exposure-number
  extraction in run(), so exposure numbers of any length work.
- Declare verbose in params_default so the contract is local instead
  of relying on cs_util.args.parse_options to inject it.

Addresses review feedback on CosmoStat#699.
- Replace hsp_map + 0 with hsp_map.copy() (healsparse supports .copy
  since at least 1.x; more explicit intent).
- Declare verbose in params_default; currently populated only by
  cs_util.args.parse_options auto-injection.
- Fix typo: "Install te cs_util" -> "Install the cs_util".

Addresses review feedback on CosmoStat#699.
- coverage_plotter, header_downloader: declare verbose explicitly in
  params_default.
- coverage_plotter: comment acknowledging that FootprintPlotter._regions
  is a private cs_util attribute; refactor to public accessor when
  cs_util exposes one.
- header_downloader: expand the vos:cfis/pitcairn comment to flag it as
  UNIONS/CFIS-specific and document override.
- build_and_plot_coverage_maps.sh: explain the BUILD_NSIDE=131072 choice
  (matches UNIONS bit-mask pixel scale, ~0.1"), and note the
  CoverageMapBuilder default of 2048 for lighter use.

Addresses review feedback on CosmoStat#699.
@cailmdaley
Copy link
Copy Markdown
Contributor

Thanks @martinkilbinger — your three fixes close out the blocking items (the bogus run_pipeline, the unreachable ImportError guard, the sp_validation→cs_util error string). I walked through the rest; current state:

Addressed on this branch:

  • field_corners_extractor.py — regex bug fixed; process_single_header and get_wcs_from_header consolidated into one helper; MegaCam HDU/pixel conventions commented (bbe53c5)
  • coverage_map_builder.pyhsp_map + 0.copy() (134a79f)
  • coverage_map_builder.pyverbose declared explicitly in params_default; header_downloader.py vospace default noted as CFIS-specific; build_and_plot_coverage_maps.sh has a comment explaining BUILD_NSIDE=131072 matches UNIONS bit-mask resolution vs the class default of 2048 for lighter-weight offline use (9a574e9)

Left as-is (with comment):

  • coverage_plotter.py:166, 241 still reaches into FootprintPlotter._regions. I added a comment at L161–163 flagging it and pointing at cs_util; the real fix is upstream (expose regions as a public accessor). Not worth duplicating the 3-region dict locally given cs_util should be the single source of truth.

Non-blocking gap to track (not in this PR):

  • No tests for the new coverage modules. Targets worth covering later: CoverageMapBuilder.median_filter and FieldCornersExtractor.process_single_header — a small synthetic header + tiny hsp map would lock in the load-bearing numerics. Happy to file an issue or pick it up after this merges, whichever you prefer.

From my side the PR is ready for a final review / merge. LMK if you want me to touch anything else.

@martinkilbinger martinkilbinger requested review from cailmdaley and removed request for sfarrens April 23, 2026 12:49
@martinkilbinger
Copy link
Copy Markdown
Contributor Author

Code Review

Overview

This PR adds a full coverage-mask pipeline for UNIONS/CFIS: five new utility classes (CcdPsfHandler, HeaderDownloader, FieldCornersExtractor, CoverageMapBuilder, CoveragePlotter), new CLI entry points, a batch shell script, and refactors the old monolithic get_ccds_with_psf.py script. The code is well-structured and consistently follows the class-based pattern used elsewhere in shapepipe.utilities.


Critical Issues

1. Broken coverage_pipeline entry point (pyproject.toml:72, coverage_run.py)

pyproject.toml registers:

coverage_pipeline = "shapepipe.coverage_run:run_pipeline"

but coverage_run.py has no run_pipeline function — only main() which returns 0 immediately. This will cause an AttributeError when the entry point is called. Either implement run_pipeline or remove the entry point.

2. Missing vos dependency (pyproject.toml, header_downloader.py:8)

import vos is at module level in header_downloader.py, but vos is not listed in pyproject.toml dependencies. A fresh install will fail with ModuleNotFoundError as soon as any coverage tool is imported.


Significant Issues

3. Debug print left in production code (ccd_psf_handler.py:831)

print("MKDEBUG stat file ", stat_file, " does not exist")

This is unconditional and will pollute stdout in production. The nearby #print("MKDEBUG ...) lines were commented out but this one was missed.

4. healsparse/skyproj as hard core dependencies (pyproject.toml:47-48)

Both are added to dependencies (i.e., required for all users), but they are only needed for the coverage pipeline. They are large, non-trivial packages. They should be moved to an optional group, e.g.:

[project.optional-dependencies]
coverage = ["healsparse", "skyproj"]

skyproj also appears redundantly in the new plot optional group.

5. Accessing private attribute FootprintPlotter._regions (coverage_plotter.py:1419-1424)

The comment acknowledges this is fragile — any rename in cs_util breaks this silently. A safer approach: let FootprintPlotter raise on an invalid region and catch it, rather than reaching into a private attribute.


Minor Issues

6. args variable shadowed in _process_parallel (field_corners_extractor.py:1961)

args = [(p, verbose) for p in todo]

Shadows the method's args parameter. Rename to e.g. worker_args.

7. Global matplotlib state mutation (coverage_plotter.py:1480-1484)

matplotlib.rc(...) and rcParams are set globally in plot_coverage_map, affecting any subsequent plots in the same session. Use with matplotlib.rc_context({...}): instead.

8. Hardcoded fallback ra_0 = 130.0 (coverage_plotter.py:1510)

The RA wraparound fallback is silently UNIONS-specific. Add a comment explaining it, or make it a configurable parameter.

9. Edge artifact in median_filter (coverage_map_builder.py:1100)

n = np.where(n >= 0, n, 0)

Invalid neighbours (boundary pixels, index = -1) are replaced with pixel 0, causing boundary pixels to over-weight their own value in the median. This could bias coverage depths at survey edges. Worth documenting the choice, or using masked arrays.

10. Misleading error message on missing plot dependency (coverage_map_builder.py:1244-1245)

print("Install the cs_util package for plotting support.")

The ImportError here would come from skyproj/FootprintPlotter, not cs_util. The message is incorrect.

11. No tests

None of the five new classes have unit tests. Given these tools produce the survey footprint mask — which feeds downstream science — even minimal tests (mock file I/O for FieldCornersExtractor, a small toy map for CoverageMapBuilder) would be valuable.


Small Nits

  • get_ccds_with_psf_method_v1_3: the n_CCD parameter is documented as unused; prefix with _ or remove it (ccd_psf_handler.py:776)
  • get_lines has no need for self; could be a @staticmethod (ccd_psf_handler.py:650)
  • check_params in CcdPsfHandler is a bare pass — fine as a stub, but could be removed
  • SGC_DEC_MIN=18 / SGC_DEC_MAX=40 in the shell script: labelling a positive-dec equatorial strip "SGC" is non-standard; a comment clarifying the UNIONS-specific definition would help

Summary

Must-fix before merge: the broken coverage_pipeline entry point (#1) and missing vos dependency (#2). The debug print (#3) and promoting healsparse/skyproj to optional dependencies (#4) are also strong candidates to address. The rest can be handled in follow-up.

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