Only use the enabled cameras for offset calculations - tickets/INSTRM-2919#104
Open
Only use the enabled cameras for offset calculations - tickets/INSTRM-2919#104
enabled cameras for offset calculations - tickets/INSTRM-2919#104Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for restricting guiding offset and focus calculations to a configured subset of “enabled” AG cameras, while still allowing all cameras to expose and populate agc_data.
Changes:
- Plumbs a one-indexed camera allowlist through acquisition/autoguide flows into
get_detected_objects()andquery_agc_data(). - Adds camera filtering to the
agc_dataquery (and to focus calculations when operating on an in-memorydetected_objectsDataFrame). - Stores enabled camera configuration on
AgCmdand passes it into field acquisition, autoguide, and focus.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/agActor/utils/focus.py | Adds optional enabled-camera filtering for focus calculations and DB queries. |
| python/agActor/utils/data.py | Adds camera filtering support to detected-object retrieval and AGC DB querying. |
| python/agActor/field_acquisition.py | Adds a cameras parameter and passes it into detected-object retrieval for offset calculations. |
| python/agActor/Controllers/ag.py | Threads camera selection through the autoguide control loop parameters into offset/focus computations. |
| python/agActor/Commands/AgCmd.py | Reads enabled cameras from actor config and forwards them to acquisition/autoguide/focus. |
| python/agActor/autoguide.py | Accepts a camera allowlist and passes it into detected-object retrieval for offset calculations. |
Comments suppressed due to low confidence (1)
python/agActor/field_acquisition.py:82
- The new
camerasparameter changes which detected objects feed into offset calculations, but the existing notebook-based tests foracquire_field()don't exercise passing a camera subset. Please add/update tests to cover thatcameras=[...]restricts the queried detected objects (and that results are consistent when some cameras are disabled).
def acquire_field(
*,
design_id: int,
frame_id: int,
visit0: int | None = None,
cameras: list[int] | None = None,
obswl: float = 0.62,
altazimuth: bool = True,
is_guide: bool = False,
**kwargs: Any,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wtgee
changed the title
enabled cameras for offset calculations - tickets/INSTRM-2917enabled cameras for offset calculations - tickets/INSTRM-2919
* Accept a list of enabled cam_id integers from `pfs_instdata`. * Use list to query the valid agc_data.
Add two new optional parameters to RADECInRShiftA: - obj_camera_id: per-detection camera ID array (1-based) - enabled_camera_ids: list of camera IDs allowed to contribute to the fit Only detections from enabled cameras enter the least-squares solve (Phases 5–6 and iterative outlier rejection). All detections are still matched and included in the returned match_result, so predicted positions for disabled cameras are preserved in the output. Thread enabled_camera_ids through calculate_offsets in FieldAcquisitionAndFocusing.py, passing filtered_detected_array[:, 0] as obj_camera_id. Defaults (None) preserve the original behaviour.
…ffsets Remove the cameras parameter from get_detected_objects so all detections are always fetched from the DB. The camera-based restriction is now applied at the fitting level via the enabled_camera_ids parameter added to calculate_offsets in the previous commit. Thread the change through the call chain: - get_detected_objects: drop cameras param and query_agc_data cameras arg - acquire_field: stop passing cameras to get_detected_objects; pass it to get_guide_offsets as enabled_camera_ids - get_guide_offsets: add enabled_camera_ids param; pass to calculate_offsets - autoguide.get_exposure_offsets: stop passing cameras to get_detected_objects; pass to get_guide_offsets as enabled_camera_ids
All detections are now always fetched for the full exposure. Camera-based restriction on the astrometric fit is handled downstream via enabled_camera_ids in calculate_offsets / RADECInRShiftA.
query_agc_data no longer accepts a cameras argument. Fetch all detections unconditionally and apply the enabledCameras filter on the DataFrame afterwards, unifying the two previously-separate code paths.
…of corrections. More safety checks for fail conditions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This gets a list of
enabledcameras frompfs_instdata(https://github.com/Subaru-PFS/pfs_instdata/pull/51/changes) and uses that to filter the detected data so that it is not used in any offset or focus caluclations.The camera will still expose and populate
agc_data.2917instead of2919.