Feature/nonsidereal aperture photometry - #131
Conversation
…s where object localisation is via fits header keywords.
…sation is via user input.
All three aperture photometry operations now take the target position as
one shape -- a list of {mjd, ra, dec} -- so the frontend has a single input
contract regardless of which is selected (agreed with the frontend team).
- AperturePhotometry (fixed/sidereal): one position, mjd carried but unused
- MovingTargetAperturePhotometry: two or more, fitted to a track
- NonSiderealAperturePhotometry: none, read from CAT-RA/CAT-DEC headers
Lets a user fold a periodic light curve (e.g. an asteroid rotation curve) by reusing the VariableStar operation's Lomb-Scargle machinery, now shared. - utils/period_analysis.py: analyze_period() whose default reproduces VariableStar's calculate_period exactly (autopower, strongest peak, analytic FAP), plus additive outputs -- a ranked candidate list including the doubled period (a single-sinusoid periodogram locks onto half the rotation period of a double-peaked body) and the sampling window function. - variable_star.py: calculate_period delegates to analyze_period; its returned period/fap are byte-identical, so behaviour is unchanged. - The three aperture photometry operations emit period analysis using the same output keys as VariableStar, so the existing frontend rendering drives both; skipped below a minimum measured-point count.# Your branch is up-to-date with 'origin/feature/nonsidereal-aperture-photometry'.
Behaviour preserving throughout: no change to any magnitude, uncertainty, comparison ensemble or diagnostic. Verified after every step against the 13 real Kleopatra frames, forcing the evolving calibration path (COMPARISON_AUTO selects the shared one on that data, so the tests alone never reach it): identical to the bit, and header mode and track mode still agree to four decimal places. The two string modes that selected behaviour are replaced by objects the operation constructs and hands down: - TargetLocator (utils/target_location.py, new) with FixedPosition, EphemerisHeaders and FittedTrack. generate_light_curve loses six parameters and gains one locator, so illegal combinations (track samples with a fixed target, a fixed target with no coordinates) stop being representable and _validate_modes has nothing left to check. - ComparisonStrategy with SharedEnsemble, EvolvingEnsemble and SharedThenEvolving. "auto" was an if/elif plus a try/except reaching into the other branch; it is now a composite holding the two it falls back between. This also closes two leaks where the pixel pipeline branched on comparison_mode == "shared" to pick the candidate coverage fraction and the drop policy: both are now properties a strategy states about itself. The same argument-bundle pattern applied in three more places: CalibrationInputs (seven parameters re-declared at four levels, 31 declarations and 43 re-passes down to 2 and 8), ComparisonMatrix (whose accessors also replace present[id][path][0] indexing at seven sites), and _SearchContext for the catalog target search.
There was a problem hiding this comment.
Pull request overview
This PR extends the aperture photometry pipeline to support non-sidereal (moving) targets by introducing target-location strategies (fixed position, per-frame ephemeris headers, or a fitted user-supplied track), adding catalog-based track refinement for sidereally tracked frames, and generalizing calibration to handle drifting fields via an “evolving” comparison ensemble. It also centralizes Lomb–Scargle period analysis so both aperture photometry and VariableStar share identical peak-period results while aperture photometry can optionally emit additional period-analysis products.
Changes:
- Add moving-target track fitting/parsing, per-frame target localization strategies, and catalog refinement of predicted positions.
- Add comparison calibration strategies (shared vs evolving vs shared-then-evolving) and integrate them into the aperture photometry pipeline.
- Add shared period-analysis utilities (including optional “extras”) and update operations/tests to use the new APIs and result shape.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| datalab/datalab_session/utils/target_track.py | New polynomial track fitting/parsing and evaluation in a tangent-plane projection. |
| datalab/datalab_session/utils/target_location.py | New TargetLocator implementations: fixed, ephemeris-header based, and fitted-track based. |
| datalab/datalab_session/utils/moving_target_search.py | New catalog-based moving-target identification/refinement around a predicted track. |
| datalab/datalab_session/utils/period_analysis.py | New shared Lomb–Scargle period analysis helpers and output formatting. |
| datalab/datalab_session/utils/light_curve_errors.py | New shared LightCurveError type for cross-module use. |
| datalab/datalab_session/utils/geometry.py | Add vectorized angular-distance helpers and nearest-neighbor distance computation. |
| datalab/datalab_session/utils/fits_metadata.py | Add ephemeris header RA/Dec parsing and exposure-midpoint MJD computation. |
| datalab/datalab_session/utils/diagnostic_images.py | Factor out diagnostic JPEG upload logic for reuse across operations. |
| datalab/datalab_session/utils/comparison_calibration.py | New shared/evolving calibration implementations and empirical error floor application. |
| datalab/datalab_session/utils/aperture_light_curve.py | Pipeline refactor to use TargetLocator + ComparisonStrategy, add candidate pool budgeting, and split diagnostics scope. |
| datalab/datalab_session/data_operations/variable_star.py | Delegate period search to shared analyze_period() to preserve existing outputs. |
| datalab/datalab_session/data_operations/aperture_photometry.py | Refactor into shared runner and add/rewire sidereal + moving-target operations. |
| datalab/datalab_session/tests/test_period_analysis.py | New tests for period analysis helpers and output formatting. |
| datalab/datalab_session/tests/test_operations.py | Update operation wiring expectations for new locator/calibration/diagnostics fields. |
| datalab/datalab_session/tests/test_moving_target_track_mode.py | New end-to-end and unit tests for fitted-track moving-target mode. |
| datalab/datalab_session/tests/test_moving_target_operations.py | New operation-layer tests validating registration and output contracts. |
| datalab/datalab_session/tests/test_moving_target_header_mode.py | New end-to-end tests for ephemeris-header moving-target mode and evolving calibration. |
| datalab/datalab_session/tests/test_aperture_photometry.py | Update existing pipeline tests for locator-based API and Phase progress reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jnation3406
left a comment
jnation3406
left a comment
There was a problem hiding this comment.
Okay this was a beast. I made a lot of comments, but lost the will to live on the comparison_calibration.py file so basically have nothing on that, or the tests. Most of my issues are with typical AI-isms like commenting too much and telling why rather than just what, using functions or complicated data structures when something simpler would be fine, and some architectural things.
I am all for the AI future of code generation but I think its super helpful to do a pass to pair down what it comes up with as well. I try to read all the comments it generates and only keep sentences which I think I would find to be helpful in the future, because there is a real fatigue for humans to read a ton of non-helpful words. I also find using less lines of code is almost always better for human readability, unless you are making the lines ungodly long or something. Maybe one day we can figure out some rules to give these AI to help them write code a little more like a human.
| Returns a calibrated light curve and diagnostic data for the frontend. | ||
| The target source, as submitted. | ||
|
|
||
| A fixed target is one position that does not change with time, so it stays the plain |
There was a problem hiding this comment.
I get suspicious when the comment of the function is longer than the code...
| 'line, which holds for a night; add a third near the middle for a series spanning ' | ||
| 'more than about half a day, since apparent tracks curve.' | ||
| ), | ||
| 'type': Format.SOURCE, |
There was a problem hiding this comment.
Previously we were just filling in target name, ra/dec for Format.SOURCE. It seems like this needs a time ra/dec rather than name, so I think maybe we should make a separate "type" for this. The frontend wizard uses this "type" to decide what input fields to render and what input to collect to send to the backend, so having something specific for the fields you need probably makes sense.
|
|
||
| def operate(self, submitter: User): | ||
| raw_track = self.input_data.get('target_track') | ||
| if not raw_track: |
There was a problem hiding this comment.
The base DataOperation _validate_inputs() method is meant to do this. I.e. call self._validate_inputs('target_track', MINIMUM_TRACK_SAMPLES). It looks like the client exception it generates says "files" in it but I think we can correct that to be more generic.
| } | ||
|
|
||
|
|
||
| def run_aperture_photometry( |
There was a problem hiding this comment.
Maybe this is just down to personal preference, but is there any reason not to make a base AperturePhotometry operation with this function in it, so the first param can be "self" rather than "operation"? Since it already relies on all the internal helper functions of an operation it seems like it would make more sense being a part of one rather than passing one in.
| Use the astropy lomb scargle to perform the periodogram analysis on the light curve | ||
| Use the astropy lomb scargle to perform the periodogram analysis on the light curve. | ||
|
|
||
| Delegates to the shared analyze_period so the aperture photometry operations reuse the same |
There was a problem hiding this comment.
I would remove the first sentence here (AI telling us what it did)
| start_mjd = (start - _MJD_EPOCH).total_seconds() / 86400.0 | ||
| else: | ||
| raise ValueError("Cannot determine an observation time: no MJD-OBS and no fallback start time.") | ||
| if not math.isfinite(start_mjd): |
There was a problem hiding this comment.
how do we get a non-finite mjd?
| # optional_float rather than header_float: a malformed EXPTIME (a string, say) should land on the | ||
| # same path as an absent or non-finite one, which is to skip the midpoint correction. Failing the | ||
| # frame instead would abort the whole light curve over a keyword worth half an exposure of motion. | ||
| exposure_seconds = optional_float(header.get("EXPTIME", 0.0)) |
There was a problem hiding this comment.
Why don't we update optional_float to take a default value in case the coercion to float fails. That way we shouldn't need the check below.
| log.setLevel(logging.INFO) | ||
|
|
||
|
|
||
| def save_diagnostic_images_to_s3( |
There was a problem hiding this comment.
I think this entire method could go back to being a helper method if we switch to using a base AperturePhotometryOperation class that we subclass... That might make more sense unless you think this is more broadly applicable than that single operation? For now the on_progress is pretty tied in to how that one operation reports progress, but maybe that will evolve over time.
If you want to keep it separate then I think we already have a s3_utils.py file where this would belong.
| between (see _discriminating_frames). | ||
|
|
||
| One object because the whole set is fixed for the search and every helper needs most of it. | ||
| `discriminating` is held as the boolean matrix rather than per-frame path lists: at 999 |
| # fraction of frames. Catalog detection near the limiting magnitude is noisy, and heterogeneous | ||
| # multi-telescope sets rarely catalog the same star in *every* frame, so requiring presence in all | ||
| # frames discards good stars and collapses the candidate pool. Only the shared ensemble can afford | ||
| # it: an evolving one must keep sparsely-detected stars, since they are what carry a drifted field. |
There was a problem hiding this comment.
I think the first sentence is enough
Summary
Extends aperture photometry to solar-system objects that move against the star field. Two new operations cover the two ways such data arrives, and the calibration is extended to survive a comparison field that turns over as the target moves.
The diff is ~4,400 lines (sorry), of which 1,902 is executable production code; the rest is tests, comments and docstrings. For scale, the sidereal aperture photometry pipeline already on
mainis 1,709 code lines. The moving-target case needs its own target model and its own calibration model, and reuses everything below that (centroiding,photometry,comparison_stars, the diagnostics renderer) unchanged.Operations
source— unchangedCAT-RA/CAT-DECper frametarget_trackThe two new operations are near identical to the fixed object aperture photometry operation with the main difference being how the target is located. Which target location strategy applies is decided by how the observation was scheduled: non-sidereal tracking gives an ephemeris in the header; sidereal tracking with short exposures gives us no record at all of where the target is so the target position has to come from the user.
All three aperture photometry operations live in
data_operations/aperture_photometry.pyand share one runner. Eachoperate()is a uses a different class construction as appropriate.Design
To perform aperture photometry in a generic way the two things that vary are the way the targets are located and the strategy for comparing field objects.
TargetLocator(utils/target_location.py) is one ofFixedPosition,EphemerisHeaders,FittedTrack.ComparisonStrategy(utils/comparison_calibration.py):SharedEnsembleEvolvingEnsembleSharedThenEvolvingLocating the target
Header mode (EphemerisHeaders) reads
CAT-RA/CAT-DECTrack mode (FittedTrack) fits a polynomial through the user's samples in a gnomonic tangent plane about the mean sample direction, which removes the RA wrap at 0h, the cos(dec) compression of RA and the pole degeneracy in one step. Two samples give a line, three or more a quadratic, capped there deliberately: extra samples sharpen the quadratic by least squares rather than raising the degree, which would risk the fit oscillating between samples.
That prediction is then used to search the frame's own catalog, rejecting sources that look stationary, and the track is refitted through the detections that move consistently. Where it fails the interpolated position stands, so a faint or uncatalogued target still yields a measurement.
Calibration across a drifting field
As the target moves the comparison cast turns over and no single ensemble spans the series.
EvolvingEnsemblesolves per-frame zero points jointly over the sparse star-by-frame matrix fromm + ZP_f = M_c, anchored to catalog magnitudes, so membership can turn over completely without causing discontinuities in the light curve. Frames sharing no stars form separate components, each anchored independently and flagged.Both strategies then get an empirical error floor measured from how well the comparison stars' own magnitudes repeat frame to frame. On the real Kleopatra series that is 12.7 mmag.
Note
Why is period analysis (~115 lines) in here? Although not strictly part of the nonsidereal photometry operation it is here because a rotation curve is the end point of, e.g measuring an asteroid, and sharing
analyze_period()with the variable star operation avoids a second periodogram implementation.Frontend changes needed
The existing Aperture Photometry operation is untouched. Its input is still
source, and its output gains one additive key.1.
target_trackon Moving Target Aperture PhotometryAt least two timed samples, declared
Format.SOURCEwithmultiple: true:The user identifies the object on two or more frames, ideally first, last and one
in the middle. RA/Dec from the frame WCS, and:
▎ mjd should be the UTC MJD of the exposure midpoint, not the start:
▎ MJD-OBS + EXPTIME / 2 / 86400. Sending the start biases every predicted
▎ position by exposure / 2 × apparent rate — sub-arcsecond for a short exposure
▎ on a slow mover, larger for a long exposure on a fast NEO.
Past roughly a 12 hour arc two samples are not enough because tracks curve; the backend emits a diagnostic recommending a third.
2. track_search_radius
Float, default 10.0 arcsec, Moving Target only. How far from the interpolated position to search each frame. Wider admits more field stars to be confused with the target.
3. New output key on all three operations
pipeline_diagnostics: string[] — series-level findings, distinct from the existing per-frame diagnostics: {basename: string[]}. This is where "Measured at the interpolated positions", "No comparison ensemble spans every frame" and "Extrapolated N frame(s) outside the sample time span" appear. Until this is rendered, a run that fell back to interpolated guesses is indistinguishable from one confirmed on every frame. The main residual risk in this PR.
4. Period analysis keys
period, fap, frequency, power match what Variable Star already emits, so the existing periodogram and fold rendering should drive these unchanged. Absent when too few points were measured. Additive: period_candidates (kind is "peak" or "double" — for a tumbling asteroid the physical rotation period is the doubled one) and window_power on the same grid as frequency.
5. Unmeasurable light-curve rows
Rows are always present, one per frame, but the calibrated magnitude and its uncertainty serialize as null where calibration failed. The plot should skip those points, not treat them as zero.
6. Operation discovery
Both new operations register themselves and appear in the wizard on deploy. Moving Target is unusable until target_track is wired; Non-Sidereal needs no new input and works immediately.
Testing
157 tests pass:
DJANGO_SETTINGS_MODULE=test_settings poetry run python manage.py testEnd to end on 13 real LCO frames of (216) Kleopatra across four sites, run
both ways: header mode and track mode (three samples, interpolated then catalog
refined) give identical magnitudes and target positions to four decimal
places, with the target located in the catalog on 13 of 13 frames.
Pre-human reviews
Claude using /review skill and copilot have already reviewed this PR and suggested changes folded in.