offset generators #2116

swalker2m · 2025-11-20T17:59:45Z

The next installment of GMOS imaging support, this PR adds offset generators to the API. The offset generation isn't used yet per se, but obviously this was a sufficiently large chunk of work already.

The offset generators offer the offset generation parameters in the API and store them in the database. The idea is that the ITC results will provide the offset count and the ODB will combine that with the other parameters and then mostly delegate the actual offset generation to the core offset generator.

N.B., I might hold off on incorporating any of this into Explore since the API is still being developed.

modules/service/src/test/scala/lucuma/odb/graphql/mutation/createObservation_GmosImaging.scala

modules/service/src/main/scala/lucuma/odb/graphql/mapping/OffsetGeneratorMapping.scala

modules/service/src/main/scala/lucuma/odb/graphql/mapping/TelescopeConfigMapping.scala

cquiroz · 2025-11-21T14:25:58Z

modules/service/src/main/scala/lucuma/odb/service/OffsetGeneratorService.scala

+              lst.pure[F]
+
+            case OffsetGeneratorInput.Grid(a, b)           =>
+              val w = (a.p.toSignedDecimalArcseconds - b.p.toSignedDecimalArcseconds).abs


we have a grid generator on lucuma-core, does it not work here? maybe we should update core if that is the case

The issue is that the core grid generator parameters require an explicit n x m count and a step size. If we used it directly, then the offset parameters will have to be explicitly adjusted as changes are made that increase or decrease the ITC exposure count result. I could be wrong but I think the idea with the grid is to cover a region as evenly as possible with exposures for any exposure count that the ITC returns.

ok, can we make the core one do that? passing a count parameter instead?

Sounds good. I'll add that as an option and make the ODB use it instead of its own implementation, but maybe after this PR?

yes, no hurry

swalker2m · 2025-11-26T19:11:51Z

modules/schema/src/main/resources/lucuma/odb/graphql/OdbSchema.graphql

  offsets: [Offset!]!

+  "Temporary, WIP -- will be moved."
+  objectOffsetGenerator: OffsetGenerator


Note the offsets field above would be removed and the generators themselves placed into new imaging observation sub-types 😢. I didn't want to break anything yet though.

Copilot

Pull request overview

This PR adds offset generators to the ODB API for GMOS imaging observations. The offset generators define how telescope offsets are generated (enumerated, grid, random, or spiral patterns) and are stored in the database but not yet used in actual sequence generation.

Key Changes

Introduced OffsetGeneratorService to manage offset generation parameters
Added database tables and views for storing offset generator configurations
Extended GraphQL schema with OffsetGenerator types and input handling
Integrated offset generators into GMOS imaging observing modes

Reviewed changes

Copilot reviewed 34 out of 35 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`OffsetGeneratorService.scala`	Core service implementing offset generation logic with enumerated, grid, random, and spiral patterns
`V1064__offset_generator.sql`	Database migration adding offset generator tables, enums, and triggers
`OffsetGeneratorInput.scala`	GraphQL input handling with validation ensuring only one generator type is specified
`OffsetGeneratorServiceSuite.scala`	Test suite covering different offset generation scenarios
`GmosImagingService.scala`	Integration layer connecting offset generators with GMOS imaging modes
`OdbSchema.graphql`	GraphQL schema additions exposing offset generator types and inputs
`OffsetMapping.scala`	Grackle mappings for offset generator GraphQL queries
Various mapping files	Updated to include offset generator views and joins

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-27T13:01:49Z

modules/service/src/main/scala/lucuma/odb/graphql/mapping/OffsetMapping.scala

-    List(
-      offsetMappingAtPath(StepRecordType / "telescopeConfig" / "offset", StepRecordView.Id)
-    )
+      offsetMappingAtPath(SpiralPath, OffsetGeneratorView.ObservationId, OffsetGeneratorView.Spiral.ObservationId, OffsetGeneratorView.Spiral.OffsetGeneratorRole),


The SpiralPath mapping is inconsistent with the other paths. Line 65 passes three column references (OffsetGeneratorView.ObservationId, OffsetGeneratorView.Spiral.ObservationId, OffsetGeneratorView.Spiral.OffsetGeneratorRole) but the RandomPath mapping on line 61 only passes two. This appears to be an error - it should match the pattern used for Random and only pass OffsetGeneratorView.Spiral.ObservationId and OffsetGeneratorView.Spiral.OffsetGeneratorRole.

Suggested change

offsetMappingAtPath(SpiralPath, OffsetGeneratorView.ObservationId, OffsetGeneratorView.Spiral.ObservationId, OffsetGeneratorView.Spiral.OffsetGeneratorRole),

offsetMappingAtPath(SpiralPath, OffsetGeneratorView.Spiral.ObservationId, OffsetGeneratorView.Spiral.OffsetGeneratorRole),

Copilot · 2025-11-27T13:01:49Z

modules/service/src/test/scala/lucuma/odb/service/OffsetGeneratorServiceSuite.scala

+        val withinSizeLimit =
+          r.values
+           .flatten
+           .map(_.offset.distance(Offset.Zero).toMicroarcseconds)
+           .forall(_ > sizeMicroarcseconds)


The logic for withinSizeLimit appears inverted. The test checks that offset distances are greater than sizeMicroarcseconds (line 233: _ > sizeMicroarcseconds), but it should verify that they are less than or equal to the size limit. Random and spiral offsets should be contained within the specified size/radius, not outside of it.

Copilot · 2025-11-27T13:01:50Z

modules/service/src/main/scala/lucuma/odb/service/OffsetGeneratorService.scala

+              val stepP = if cols <= 2 then w else w / (cols - 1) // arcseconds
+              val stepQ = if rows <= 2 then h else h / (rows - 1) // arcseconds


The edge case handling for grid step calculation has an off-by-one issue. When cols <= 2 or rows <= 2, the step size is set to the full width/height. However, for exactly 2 columns/rows, this will place both points at the same position (corner). The condition should be cols <= 1 and rows <= 1 to correctly handle 2 points spanning the full width/height.

Suggested change

val stepP = if cols <= 2 then w else w / (cols - 1) // arcseconds

val stepQ = if rows <= 2 then h else h / (rows - 1) // arcseconds

val stepP = if cols <= 1 then w else w / (cols - 1) // arcseconds

val stepQ = if rows <= 1 then h else h / (rows - 1) // arcseconds

w = w / (2 - 1) and h = h / (2 - 1)

Copilot · 2025-11-27T13:01:50Z

modules/service/src/main/scala/lucuma/odb/service/OffsetGeneratorService.scala

+              List.empty[TelescopeConfig].pure[F]
+
+            case OffsetGeneratorInput.Enumerated(lst)      =>
+              LazyList.continually(lst).flatten.take(count).toList.pure[F]


The Enumerated case doesn't respect the guideState parameter. Each TelescopeConfig in lst already has its own guiding value, but the method signature suggests the passed guideState should be applied. This inconsistency could lead to incorrect guiding behavior. Consider either documenting that enumerated offsets preserve their own guide states, or apply the parameter's guide state to all returned configs.

Suggested change

LazyList.continually(lst).flatten.take(count).toList.pure[F]

LazyList.continually(lst)

.flatten

.take(count)

.map(tc => tc.copy(guideState = guideState))

.toList

.pure[F]

Copilot · 2025-11-27T13:01:50Z

modules/service/src/test/scala/lucuma/odb/graphql/mutation/cloneObservation.scala

    }
  }

+  test("close GMOS imaging observation preserves offset generator"):


Typo in test name: "close" should be "clone".

cquiroz

Looks like an amazing work, I didn't check the tests but the code looks great. I left a few comments but none is a stopper

cquiroz · 2025-11-27T12:59:38Z

modules/schema/src/main/resources/lucuma/odb/graphql/OdbSchema.graphql

  offsets: [OffsetInput!]

+  "Temporary, WIP -- will be moved."
+  objectOffsetGenerator: OffsetGeneratorInput


What's the difference between object and sky offsets?

Sky offsets are used to calibrate the science data. I think they measure sky background to contrast with the science, but Andy or Bryan would have a real explanation. For our purposes, some imaging sequences will come with "sky" positions sandwiched before/after the science data. These are typically far enough away that they are cannot be guided (I believe) and may have a different pattern than the science offsets.

cquiroz · 2025-11-27T13:01:29Z

modules/schema/src/main/resources/lucuma/odb/graphql/OdbSchema.graphql

+# Defines a region of the sky using two corners.  Exposures are
+# then distributed across this region as evenly as possible.
+type GridOffsetGenerator {
+  cornerA: Offset!


are these offsets from the base?

cquiroz · 2025-11-27T13:02:50Z

modules/service/src/main/resources/db/migration/V1064__offset_generator.sql

+      array_remove(array_agg(c_filter ORDER BY c_filter), NULL) AS c_filters
+    FROM t_gmos_south_imaging_filter
+    WHERE c_version = 'current'
+    GROUP BY c_observation_id


Should we drop the existing offset columns/table?

Yes, in a future PR there will be updates to the existing imaging offset positions in the API and database.

cquiroz · 2025-11-27T13:03:28Z

modules/service/src/main/scala/lucuma/odb/graphql/input/OffsetGeneratorInput.scala

+  case object NoGenerator extends OffsetGeneratorInput
+
+  case class Enumerated(
+    values: List[TelescopeConfig]


Should the be nonempty?

I didn't see why it should be limited if an empty list works but now that you mention it, that just gives another way to have a NoGenerator option. I think I'll change it.

swalker2m · 2025-11-27T13:08:28Z

modules/service/src/main/scala/lucuma/odb/service/OffsetGeneratorService.scala

+            case OffsetGeneratorInput.Grid(a, b)           =>
+              val w = (a.p.toSignedDecimalArcseconds - b.p.toSignedDecimalArcseconds).abs
+              val h = (a.q.toSignedDecimalArcseconds - b.q.toSignedDecimalArcseconds).abs
+              val aspectRatio = w / h


I need to handle the case where the q values are the same (and thus h is 0) before this can be merged. I think I'll rename it uniform to distinguish it from the grid option. I'll add it to core as well for eventual use from the ODB.

github-actions bot added the skip-sc Don't complete the associated story when merged. label Nov 20, 2025

mergify bot added schema sequence labels Nov 20, 2025

swalker2m commented Nov 20, 2025

View reviewed changes

modules/service/src/test/scala/lucuma/odb/graphql/mutation/createObservation_GmosImaging.scala Show resolved Hide resolved

swalker2m commented Nov 20, 2025

View reviewed changes

modules/service/src/main/scala/lucuma/odb/graphql/mapping/OffsetGeneratorMapping.scala Outdated Show resolved Hide resolved

swalker2m commented Nov 20, 2025

View reviewed changes

modules/service/src/main/scala/lucuma/odb/graphql/mapping/TelescopeConfigMapping.scala Outdated Show resolved Hide resolved

cquiroz reviewed Nov 21, 2025

View reviewed changes

tgodzik mentioned this pull request Nov 24, 2025

Could not find a line from stack traces scalameta/metals#7974

Open

swalker2m force-pushed the sc-4028-gmos-imaging-03 branch 3 times, most recently from 3733a99 to 47f2db7 Compare November 25, 2025 12:08

mergify bot added calibrations obscalc labels Nov 25, 2025

swalker2m force-pushed the sc-4028-gmos-imaging-03 branch 4 times, most recently from 81e88a1 to 40ea91a Compare November 26, 2025 18:59

swalker2m marked this pull request as ready for review November 26, 2025 19:07

swalker2m commented Nov 26, 2025

View reviewed changes

cquiroz requested a review from Copilot November 27, 2025 12:58

Copilot started reviewing on behalf of cquiroz November 27, 2025 12:59 View session

Copilot finished reviewing on behalf of cquiroz November 27, 2025 13:00

Copilot AI reviewed Nov 27, 2025

View reviewed changes

cquiroz approved these changes Nov 27, 2025

View reviewed changes

swalker2m commented Nov 27, 2025

View reviewed changes

swalker2m added 5 commits November 27, 2025 17:49

offset generators

f9508d8

fix trigger

1863a3d

enum startup check

d9c2866

clone

61e7e7a

update

c068539

swalker2m added 6 commits November 27, 2025 17:49

cleanup

216041d

generation

23ebbda

scalacheck-effect-munit ?

a380e65

co-pilot review suggestions

22a1159

enumerated - non-empty list

022e0ef

switch to uniform, update calculation

6f52dc1

swalker2m force-pushed the sc-4028-gmos-imaging-03 branch from 40ea91a to 6f52dc1 Compare November 27, 2025 20:50

swalker2m merged commit 895c11d into main Nov 28, 2025
25 checks passed

swalker2m deleted the sc-4028-gmos-imaging-03 branch November 28, 2025 11:52

	offsetMappingAtPath(SpiralPath, OffsetGeneratorView.ObservationId, OffsetGeneratorView.Spiral.ObservationId, OffsetGeneratorView.Spiral.OffsetGeneratorRole),
	offsetMappingAtPath(SpiralPath, OffsetGeneratorView.Spiral.ObservationId, OffsetGeneratorView.Spiral.OffsetGeneratorRole),

		val stepP = if cols <= 2 then w else w / (cols - 1) // arcseconds
		val stepQ = if rows <= 2 then h else h / (rows - 1) // arcseconds

offset generators #2116

offset generators #2116

Uh oh!

Conversation

swalker2m commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

cquiroz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swalker2m commented Nov 20, 2025 •

edited

Loading