-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright (c) 2016-2023 Association of Universities for Research in Astronomy, Inc. (AURA) | ||
// For license information see LICENSE or https://opensource.org/licenses/BSD-3-Clause | ||
|
||
package lucuma.core.model.syntax | ||
|
||
import cats.data.NonEmptyList | ||
import cats.syntax.all.given | ||
import lucuma.core.math.Coordinates | ||
import lucuma.core.model.SiderealTracking | ||
|
||
import java.time.Instant | ||
import scala.annotation.targetName | ||
|
||
trait ToTrackingOps { | ||
extension(trackings: NonEmptyList[SiderealTracking]) | ||
// We calculate the coordinates at a given time by doing PM | ||
// correction of each tracking and then finding the center | ||
@targetName("TrackingList_centerOfAt") | ||
def centerOfAt(i: Instant): Option[Coordinates] = | ||
trackings | ||
.map(_.at(i)) | ||
.sequence | ||
.map(nel => Coordinates.centerOf(nel.toList)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me see if I understand. Right now, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi sorry, that wasn't clear. I expect we'll have a 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 commentThe reason will be displayed to describe this comment to others. Learn more. I was just thinking that maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There are a lot of those in lucuma-schemas for creating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍
Agree, we shouldn't get into that right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
@targetName("TrackingList_centerOf") | ||
def centerOf: Coordinates = | ||
val coords = trackings.map(_.baseCoordinates) | ||
Coordinates.centerOf(coords.toList) | ||
|
||
} | ||
|
||
object tracking extends ToTrackingOps |
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 anOption
and complicate a lot of the code in Explore. I had originally made this return anOption[ObjectTracking]
and also had afromAsterismUnsafe
. 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 offromTarget
also makes this assumption that the target is sidereal.An alternative would be to return a
ConstantTracking
(adding it back) which always returns aCoordinates.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.