-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we snhould only allow sidereals here right in the signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Nonsidereal
s.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Input
s (.toInput
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 pending CI
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.