ENH: Add AltAzTarget and support time-dependent targets#634
ENH: Add AltAzTarget and support time-dependent targets#634michaelbaisch wants to merge 4 commits intoastropy:mainfrom
Conversation
- Add AltAzTarget for targets defined in horizontal coordinates at a fixed EarthLocation, evaluated to time-dependent ICRS SkyCoord when needed; support vacuum (pressure=0) and apparent (pressure>0) interpretations via stored atmospheric parameters. - Export AltAzTarget and make get_skycoord(time-aware): accept `times`, evaluate time-dependent targets via get_skycoord(times), and broadcast coords. - Pass `time` into get_skycoord from Observer._preprocess_inputs to enable time-dependent target evaluation during constraint calculations. - Update Constraint.__call__ to delegate gridding and time-dependent evaluation to Observer._preprocess_inputs. - In scheduling add `target_type`/`target_info` columns in Schedule.to_table. - Add tests.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #634 +/- ##
==========================================
+ Coverage 72.90% 74.60% +1.69%
==========================================
Files 14 14
Lines 1849 1953 +104
==========================================
+ Hits 1348 1457 +109
+ Misses 501 496 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…SkyCoord targets when checking for multiple targets
m4cd4r4
left a comment
There was a problem hiding this comment.
Solid addition to astroplan. The AltAzTarget class fills a real gap for users working with local pointing directions rather than celestial coordinates. Test coverage is thorough (208 lines).
A few things I noticed:
Breaking change in Schedule.to_table():
The ra and dec columns are replaced with target type and target info. Any downstream code that accesses table['ra'] or table['dec'] will break silently. Would it be possible to keep the old columns (populated where applicable) alongside the new ones? Or add a deprecation warning for one release cycle?
Time-dependent detection heuristic:
The _is_time_dependent check (has get_skycoord callable and no coord attribute) is duck-typing based. If someone later adds a get_skycoord method to FixedTarget, this logic breaks silently. A dedicated is_time_dependent property on the Target base class would be more explicit and safer against future changes.
Atmospheric parameter defaults:
The docstring says pressure defaults to 0 hPa (vacuum), but the code passes pressure=None to AltAz. Depending on the astropy version, AltAz(pressure=None) could use a different default. Setting pressure = 0 * u.hPa explicitly in __init__ would guarantee vacuum behavior regardless of astropy version.
Minor:
- The
except Exceptionblocks in_format_target_infoare broad. Narrowing toAttributeErrororConvertErrorwould avoid masking real bugs. - The error message when
times=Nonefor a time-dependent target could mention that the caller needs to providetimes(currently just raises a bareValueError).
The refactoring of _preprocess_inputs to handle time-dependent gridding is clean, and the score_key rounding in the scheduler is a good catch. Nice work.
times, evaluate time-dependent targets via get_skycoord(times), and broadcast coords.timeinto get_skycoord from Observer._preprocess_inputs to enable time-dependent target evaluation during constraint calculations.target_type/target_infocolumns in Schedule.to_table.#566 was updated to be compatible with this PR.