Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add an ObjectTracking for tracking asterisms of more than one target #778

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Add an ObjectTracking for tracking asterisms of more than one target #778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
ThisBuild / tlBaseVersion := "0.83"
ThisBuild / tlBaseVersion := "0.84"
ThisBuild / tlCiReleaseBranches := Seq("master")
ThisBuild / githubWorkflowEnv += "MUNIT_FLAKY_OK" -> "true"

Expand Down
17 changes: 13 additions & 4 deletions modules/core/shared/src/main/scala/lucuma/core/model/ObjectTracking.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
package lucuma.core.model

import cats.Eq
import cats.data.NonEmptyList
import cats.derived.*
import cats.syntax.all.*
import lucuma.core.math.Coordinates
import lucuma.core.model.syntax.tracking.*
import lucuma.core.util.NewType

import java.time.Instant
Expand All @@ -24,17 +26,24 @@ sealed trait ObjectTracking derives Eq:
def baseCoordinates: Coordinates

object ObjectTracking:
case class ConstantTracking(coord: Coordinates) extends ObjectTracking derives Eq:
def at(i: Instant): Option[CoordinatesAtVizTime] = CoordinatesAtVizTime(coord).some
def baseCoordinates: Coordinates = coord

case class SiderealObjectTracking(tracking: SiderealTracking) extends ObjectTracking derives Eq:
def at(i: Instant): Option[CoordinatesAtVizTime] =
tracking.at(i).map(CoordinatesAtVizTime(_))
def baseCoordinates: Coordinates = tracking.baseCoordinates

def const(coord: Coordinates): ObjectTracking = ConstantTracking(coord)
case class SiderealAsterismTracking(trackings : NonEmptyList[SiderealTracking]) extends ObjectTracking derives Eq:
def at(i: Instant): Option[CoordinatesAtVizTime] =
trackings.centerOfAt(i).map(CoordinatesAtVizTime(_))
def baseCoordinates: Coordinates = trackings.centerOf

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 =
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

val trackingList = targets.toList.traverse(Target.sidereal.getOption).flatMap(NonEmptyList.fromList)
trackingList.fold(sys.error("Only sidereal targets supported")) { sidereals =>
if (sidereals.length === 1) SiderealObjectTracking(sidereals.head.tracking)
else SiderealAsterismTracking(sidereals.map(_.tracking))
}
32 changes: 32 additions & 0 deletions modules/core/shared/src/main/scala/lucuma/core/model/syntax/TrackingOps.scala
View file
Open in desktop
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))
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

@toddburnside toddburnside Aug 9, 2023 •
edited
Loading

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?

Copy link
Contributor

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 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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 Inputs (.toInput).

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.


@targetName("TrackingList_centerOf")
def centerOf: Coordinates =
val coords = trackings.map(_.baseCoordinates)
Coordinates.centerOf(coords.toList)

}

object tracking extends ToTrackingOps
2 changes: 1 addition & 1 deletion modules/core/shared/src/main/scala/lucuma/core/model/syntax/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
package lucuma.core.model

package object syntax {
object all extends ToNonNegDurationOps
object all extends ToNonNegDurationOps with ToTrackingOps
}