gemini-hlsw / gem

prototype postgres back-end for ocs
Other
15 stars 5 forks source link

Ephemeris in Nonsidereal Target #176

Closed swalker2m closed 6 years ago

swalker2m commented 6 years ago

At the moment the target model includes a Nonsidereal Track instance that associates a unique ephemeris key with a map of ephemeris data.

final case class Nonsidereal(ephemerisKey: EphemerisKey, ephemerides: Map[Site, Ephemeris]) extends Track {

  override def at(time: Instant, s: Site): Option[Coordinates] =
    ephemeris(s).flatMap(_.get(InstantMicros.truncate(time)).map(_.coord))

  ...
}

When such a target is read from or written to the database the ephemeris data is not included. We'd like to avoid necessarily having to read ephemeris data every time we read targets, and yet we need the information to compute coordinates.

I'm opening this issue to discuss options for how to proceed.

tpolecat commented 6 years ago

The issue I keep thinking about is that when we load an ephemeris we're going to want to provide a timespan and perhaps a resolution, to minimize the amount of data we're moving. And in general we can't know this statically … a high-res ephemeris for today and a long-range one for next semester will have the same type. So if we frame the question as "how much do we load?" rather than "do we load it at all?" the current setup works. When you load a target you have to specify a date range and if the range is empty then ¯\(ツ)/¯ ok your ephemeris is empty.

Re: storing I think we probably don't ever want to store Horizons ephemerides since they're owned by the robot, but I guess it makes sense to save custom ones when we store the target.

What do you think? I'm not really at a point of clarity yet.

swalker2m commented 6 years ago

If a TargetEnvironment is part of an Observation this implies that we need to specify a time range (or None if we don't care about coordinates) when we load an observation as well. Does that sound right?

tpolecat commented 6 years ago

I think this is right, although maybe we just want to pass an empty range instead … offhand I can't think of a good reason to distinguish Some(emptyRange) from None.

swalker2m commented 6 years ago

One issue with this approach is that you have to understand what you're getting with equality comparisons. You can have two nonsidereal targets with the same id and yet they will not be considered equal unless you happen to have loaded the same set of ephemeris data for each.

I'm not sure whether this should be considered surprising or incorrect, but it took me a while to figure out what was going wrong in test cases.

swalker2m commented 6 years ago

And of course, the equality comparison issue bubbles up into the target environment and observations that contain them. Two observations may be identical in every way except that one has a wider range of ephemeris for a particular nonsidereal target. In that case, they aren't really the same observation according to ===.

tpolecat commented 6 years ago

At one level this seems ok … other than testing it doesn't seem like we're going to be doing these structural comparisons very often. On the other hand maybe it would simplify things to unmoor the ephemeris from the target model and just store the key. We could have an EphemerisSet or something that you get back when you fetch targets, and you have to pass it back to the target to get the coordinates. We could also do some type level stuff and refine the observation over the date range where its target coordinates are known, which would make the comparisons not typecheck if they had different ranges. I think this would end up being complicated though.

swalker2m commented 6 years ago

maybe it would simplify things to unmoor the ephemeris from the target model and just store the key

I was considering this idea as I was dealing with the equality issues. I have a version more or less working where you specify a site and time range whenever you load a target (or an observation with which it is associated), but it doesn't feel right. The concept of where a target is at a given time seems separate from the target's fundamental identity. You're certainly not always interested in where the target is when you load an observation and it seems like a hassle to have to provide a bogus time range in that case. I guess. I'm not totally convinced either way.

I was not thinking of getting an EphemerisSet when you fetch a target but rather keeping the target tracking completely independent of loading targets. If you need to know where the target is, you'd then need to get some sort of Tracking object with an at method like today's Track (which I guess would be renamed and be left as just the sum type). This would potentially require a query for non-sidereal targets but the API would be the same regardless:

TrackingDao.select(t: Target, s: Site, start: Timestamp, end: Timestamp): ConnectionIO[Tracking]

I think I'll try a version along these lines if it doesn't sound too stupid.

tpolecat commented 6 years ago

This seems reasonable to me. We often need to know where targets are but keeping it orthogonal will make it easier to reason about so 👍

swalker2m commented 6 years ago

I think this is settled, for now, with #204