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.

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

WIP: observing mode group #1470

Merged
merged 14 commits into from
Nov 12, 2024
Merged

WIP: observing mode group #1470

merged 14 commits into from
Nov 12, 2024

Conversation

swalker2m
Copy link
Contributor

@swalker2m swalker2m commented Nov 5, 2024

This is a work-in-progress attempt to add observation grouping by observing mode in a way similar to the constraint set grouping. I'd like feedback on whether this looks feasible.

Constraint set grouping is a much simpler case case because observing modes:

  • are heterogeneous and live in separate tables (t_gmos_north_long_slit vs t_gmos_south_long_slit)
  • have default values that are used when explicit values are not provided

In particular the last point is problematic because the default binning calculation is detailed. Currently we compute the defaults in code, but in order to group observing modes regardless of whether the binning is explicitly provided or supplied by default, I need to get default binning into the long slit tables. How to do that is the question. In this version the calculation is done in plpgsql but other alternatives might be:

  • Carefully update all the mutations that touch relevant fields and explicitly redo the calculation and update via Scala code.
  • Create a channel for updates to anything that would impact the default binning, respond to updates in Scala code.

What do we think best?

@mergify mergify bot added the service label Nov 5, 2024
@swalker2m
Copy link
Contributor Author

swalker2m commented Nov 5, 2024

An example of the mode key:

lucuma-odb=# select c_observation_id, c_mode_key from v_observation where c_mode_key IS NOT NULL LIMIT 10;
 c_observation_id |                                                  c_mode_key                                                   
------------------+---------------------------------------------------------------------------------------------------------------
 o-106            | p-103:gmos_south_long_slit:R150_G5326:None:LongSlit_2_00:695000:One:One:Slow:Low:FullFrame:0,20,-20:0,15,-15
 o-107            | p-104:gmos_north_long_slit:R400_G5305:None:LongSlit_1_50:700000:One:One:Slow:Low:FullFrame:0,8,-8:0,15,-15
 o-129            | p-100:gmos_north_long_slit:R831_G5302:None:LongSlit_1_00:890000:One:One:Slow:Low:FullFrame:0,5,-5:0,15,-15
 o-12a            | p-100:gmos_south_long_slit:R400_G5325:None:LongSlit_0_25:591000:One:One:Slow:Low:FullFrame:0,8,-8:0,15,-15
 o-12b            | p-104:gmos_south_long_slit:B480_G5327:None:LongSlit_1_00:555000:One:One:Slow:Low:FullFrame:0,5,-5:0,15,-15
 o-12e            | p-106:gmos_north_long_slit:R150_G5308:None:LongSlit_5_00:695000:One:One:Slow:Low:FullFrame:0,20,-20:0,15,-15
 o-14a            | p-100:gmos_north_long_slit:R831_G5302:None:LongSlit_1_00:600000:One:One:Slow:Low:FullFrame:0,5,-5:0,15,-15
 o-14d            | p-100:gmos_north_long_slit:B1200_G5301:None:LongSlit_5_00:600000:One:One:Slow:Low:FullFrame:0,5,-5:0,15,-15
 o-14e            | p-100:gmos_north_long_slit:R400_G5305:None:LongSlit_0_25:600000:One:One:Slow:Low:FullFrame:0,8,-8:0,15,-15
 o-151            | p-103:gmos_north_long_slit:R150_G5306:OG515:LongSlit_0_25:775000:One:One:Slow:Low:FullFrame:0,20,-20:0,15,-15
(10 rows)

CREATE OR REPLACE FUNCTION extract_source_profile_summary(
src_profile jsonb
)
RETURNS source_profile_summary AS $$
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs testing but we would need to extract the source profile type (point, uniform, or gaussian) from the JSON blob along with the fwhm if gaussian. This information is needed to compute the object size for the binning calculation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my fault … I was concerned that designing a table structure for source profiles would have driven me mad.

-- Set the default binning.
CREATE OR REPLACE PROCEDURE set_gmos_long_slit_default_binning(
oid d_observation_id
) AS $$
Copy link
Contributor Author

@swalker2m swalker2m Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to calculate the default binning in order to group observing modes. Usually the binning is not explicitly set but we need to know what binning will be used to group correctly. As you can see, this calculation is rather involved. It makes me nervous to put so much logic into plpgsql. Other options that occur to me:

  • Carefully update all the mutations that touch relevant fields and explicitly redo the calculation and update via Scala code.
  • Create a channel for updates to anything that would impact the default binning, respond to updates in Scala code.

What would be best?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the right way to do it, in the sense that it's never stale. If we rely on application logic to keep it up to date we will eventually hit situations where it's wrong. Having said that, we're relying on channel events to deal with calibrations and may end up doing the same for observation warnings and workflow status. So it's hard to say. There will always be problems in this middle ground where a PL/SQL solution is doable but starts to feel like too much. My instinct right now is to just go with it since it works.

CASE WHEN o.c_observing_mode_type = 'gmos_north_long_slit' THEN mode_gnls.c_mode_key END,
CASE WHEN o.c_observing_mode_type = 'gmos_south_long_slit' THEN mode_gsls.c_mode_key END,
NULL
) AS c_mode_key
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new bit in the view, but I suspect that doing this calculation in the view is not a good idea. Instead, I think it probably would need to be done via triggers on updates to the c_mode_key in t_gmos_north_long_slit and t_gmos_south_long_slit. That way we can index on this value for fast joins.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying it with a real t_observation.c_mode_key column updated via a trigger :-/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably fine to do it in the view if it's not being selected 100% of the time, but the safe assumption seems to be that Explore always selects absolutely everything.

@swalker2m
Copy link
Contributor Author

I'll mark this as "ready" but recognizing that feedback on the binning calculation may necessitate redoing that chunk of it.

@swalker2m swalker2m marked this pull request as ready for review November 12, 2024 14:59
Copy link
Member

@tpolecat tpolecat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have thought about all the issues that come to mind for me, and I'm also not really sure what the right approach is. But it seems to be correct and will probably perform fine once it's a triggered computation, so in the spirit of progress I think it's good to go. 👍

@swalker2m swalker2m merged commit 6745a2f into main Nov 12, 2024
6 checks passed
@swalker2m swalker2m deleted the obs-mode-grouping branch November 12, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants