Add an ObjectTracking for tracking asterisms of more than one target
#778
Conversation
|
|
||
| def fromTarget(target: Target): ObjectTracking = target match | ||
| case t: Target.Sidereal => SiderealObjectTracking(t.tracking) | ||
| case _ => sys.error("Only sidereal targets supported") | ||
|
|
||
| def fromAsterism(targets: NonEmptyList[Target]): ObjectTracking = |
There was a problem hiding this comment.
I wonder if we snhould only allow sidereals here right in the signature
There was a problem hiding this comment.
We could do that, but it would of course require a lot more code every time one is instantiated to make a NonEmptyList[SiderealTarget], which will of course add in an Option and complicate a lot of the code in Explore. I had originally made this return an Option[ObjectTracking] and also had a fromAsterismUnsafe. But, my thought was that when it is possible for us to get non-sidereals in an asterism, we could update this method to handle non-sidereals and not change any other code. The current implementation of fromTarget also makes this assumption that the target is sidereal.
An alternative would be to return a ConstantTracking (adding it back) which always returns a Coordinates.Zero if the asterism has non-sidereals in it. This would avoid a possible exception, but make it harder to find when sidereals are added.
There was a problem hiding this comment.
Yeah, we'll want to add non-sidereal support at some point. Until then everything will be sidereal anyway.
| trackings | ||
| .map(_.at(i)) | ||
| .sequence | ||
| .map(nel => Coordinates.centerOf(nel.toList)) |
There was a problem hiding this comment.
We only have SiderealTracking at the moment, but presumably when we add NonSiderealTracking we might make a Tracking interface with the at method and have the two implement it. Then a Tracking companion with centerOf to parallel Coordinates.centerOf? I suppose we could do all of that now in fact, even if SiderealTracking is the only implementation. Does that make sense?
There was a problem hiding this comment.
Let me see if I understand. Right now, Target.Sidereal has a SiderealTracking member which has the data from the c_sid_* columns in t_target. That SiderealTracking has the at method on it. Are you suggesting we add a NonSiderealTracking member to Target.NonsiderealTarget that contains the c_nsid fields? ie. Change the structure of Target.Nonsidereal?
There was a problem hiding this comment.
I have no idea how tracking of Nonsidereals will be done. Maybe this would require additional information not in the table yet? And you're just thinking of adding an empty Tracking interface to Target.Nonsidereal now?
There was a problem hiding this comment.
Hi sorry, that wasn't clear. I expect we'll have a NonsiderealTracking member at some point and it will have to do the ephemeris lookup from the database. I'm not suggesting we change Target.Nonsidereal now though or that we implement the at method for nonsidereals. Just to (perhaps) promote the at method to a Tracking interface that SiderealTracking can implement now. That way we could provide a Tracking.centerOf to match Coordinates.centerOf. For that matter we could skip all of that and just a make Target.centerOf that throws an exception for now for Nonsidereals.
Mostly I think I'd just never noticed an extension method that only applies to a particular type of collection like that. It's probably fine though.
There was a problem hiding this comment.
I was just thinking that maybe Target should have the at method, which Sidereal could defer to it's SiderealTracking, and Nonsidereal could throw. It's not clear to me that Nonsidereal should have a Tracking. The Tracking would presumably require data that is directly in Nonsidereal? Although I guess we could change that.
There was a problem hiding this comment.
Mostly I think I'd just never noticed an extension method that only applies to a particular type of collection like that. It's probably fine though.
There are a lot of those in lucuma-schemas for creating Inputs (.toInput).
There was a problem hiding this comment.
There are a lot of those in lucuma-schemas for creating Inputs (.toInput).
👍
It's not clear to me that Nonsidereal should have a Tracking. The Tracking would presumably require data that is directly in Nonsidereal? Although I guess we could change that
Agree, we shouldn't get into that right now. Target.at might be fine or just leaving it as is would also be fine.
There was a problem hiding this comment.
Target.at would have the advantage of only requiring the change there when we support non-sidereals. I can do that quickly.
There was a problem hiding this comment.
On second thought, looking at that raises a bunch of questions that aren't easy to answer right now. I think I'll leave this as is if someone will approve it. We can change ObjectTracking.fromAsterism at some point when we know more about how we're going to handle non-sidereals.
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #778 +/- ##
==========================================
- Coverage 78.84% 78.71% -0.14%
==========================================
Files 113 113
Lines 1494 1494
Branches 3 3
==========================================
- Hits 1178 1176 -2
- Misses 316 318 +2 ☔ View full report in Codecov by Sentry. |
This is used to improve asterism tracking in Explore for guide stars and other things. It will also be used in ODB for the guide stars.