v0.6.0#8
Conversation
…n_parameters.json from being overwritten
|
This PR got a bit larger than expected. Summary of changes:
|
NCastro-Segura
left a comment
There was a problem hiding this comment.
Hey, thanks for putting this together! The PR description undersells it a bit — there are some genuinely important fixes buried in here alongside the docs work, so good to see them land.
A few things I wanted to flag:
Forced photometry fix — this one's subtle but critical. The catalog coordinates weren't going through the per-image affine transform, so results were effectively pinned to the reference frame regardless of the actual image offset. The fix looks correct. One thing worth considering: users who ran forced photometry with earlier versions will have silently wrong light curves. Might be worth a note in the changelog so they know to rerun their reductions.
Image grouping by camera + filter — good catch. Grouping by filter alone was always going to be a problem for a multi-camera instrument where all three cameras can share the same filter name. The camera_and_filter_key() approach is clean and consistent with how the rest of the code handles this.
rebin() simultaneity and gap handling — both fixes make sense. Using t_ref as the common time_bin_start across all keys is the right call for multi-band work where you need the time grids to align. And removing gap bins via GTIs rather than padding with zeros is the scientifically correct behaviour. One minor suggestion: it'd be worth calling out the simultaneity guarantee explicitly in the rebin() docstring — it's a non-obvious property that users doing multi-band differential photometry will care about.
FITS file discovery — pragmatic change, I like it. One small thing: glob('*') will also match subdirectories, and fits.open() on a directory throws an exception that gets caught and emits a warning. It won't break anything, but the warning message will be confusing. A .is_file() check before attempting to open would clean that up.
Reducer parameter safeguard — solid addition. The json.dumps comparison works fine for now, though it's worth keeping in mind that if floating-point parameters get added in the future, formatting differences could theoretically cause false positives.
MEXMAN pixel scale — glad this got corrected. Was this from an updated instrument spec or measured on-sky?
ReadTheDocs Python 3.14 — just double-checking: is 3.14 available yet on the ubuntu-22.04 RTD image? It was still prerelease last I checked. Worth verifying the docs build doesn't break after merge.
Nothing here is a blocker. Happy to approve once you've had a chance to look at the .is_file() suggestion and the changelog note for the forced photometry fix.
… expanded some docstrings
…ossible for all telescopes; calibration images are now rebinned as they are opened to avoid resolution differences between different calibrations
…y be useful for faint/eclipsing sources where forced photometry may be required
…e resulting ECSV files
|
Cheers, @NCastro-Segura! I've implemented your suggestions for FITS file discovery and the The MEXMAN pixel scale came from observers at OAN-SPM. I don't think there are any issues with Python 3.14 support any more. Indeed, the dev page of the docs appears to work perfectly: https://opticam.readthedocs.io/en/dev/ |
Finishing touches for v0.6.0.
Reducerfrom being instantiated if the specifiedout_directoryalready contains areduction_parameters.jsonfile that does not match theReducerinstance