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

Added new QGRaphicsItems (FieldOfView, Centroid) #6

Merged
merged 21 commits into from
Jan 28, 2025
Merged

Conversation

javierggt
Copy link
Contributor

@javierggt javierggt commented Nov 21, 2024

Description

The purpose of this PR is to:

  • enable a simple real-time/telemetry use case together with aca_view.
  • add a feature to automatically call proseco in the standard aperoll use.

This PR makes the following changes:

  • Adds three graphics items:
    • Centroid. Markers for centroids from telemetry.
    • CameraOutline. A graphics item depicting the edges of the CCD and the quadrant boundaries.
    • FieldOfView. This is an item that contains a list of centroid items and a camera outline. It would be easy to also associate a catalog with it.
  • Adds a function star_field_position that is used by all items to recalculate their position when the scene attitude changes.
  • Adds a context menu to hide/show FOV, catalog and centroids.
  • Puts all switches that determine StarField behavior into a dataclass of type StarFieldState. The idea is that this is the one place to find all run-time switches that the user can change.
  • Adds an auto-proseco feature, which causes proseco to be run whenever the parameters change
  • Changes StarView so StarView.catalog is never None, and adds Catalog.reset instead.

Notes about testing/reviewing this PR

I would like to restrict this PR to the items mentioned above. I tried to keep the changes to the user interface as minimal as possible while still allowing me to toggle the item's visibility. This PR is not intended to make major changes in the UI. The main change is the addition of the context menu, which is not conspicuous. It should be functional though:

  • one should be able to run in real-time and browse the star field (no proseco catalog)
  • one should be able to evaluate difficult star fields the usual way and proseco should run automatically.
  • one should be able to toggle visibility of the new items and they should behave as expected (centroids updated from telemetry, etc).

One thing to note is the StarPLot.set_onboard_attitude function.

Requirements

This requires the latest agasc master (after sot/agasc/pull/193).

For real-time/telemetry use, use sot/aca_view/pull/197

Interface impacts

Testing

Unit tests

  • No unit tests
  • Mac
  • Linux
  • Windows

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

The usual aperoll command to inspect catalogs

aperoll input.json

Usage within aca_view to inspect telemetry. in this case, I tried: showing/hiding different items, dragging the display, opening the star view before there is any telemetry. Note that the "auto-proseco" feature does not work in this case.

aca_view --obsid 42979 --stop 500

- changed it so StarView.catalog is never None, and added Catalog.reset instead.
- Added context menu to hide/show FOV, catalog and centroids.
- Added auto-proseco feature
@javierggt javierggt changed the title Added new QGRaphicsItems (FieldOfView, Centroid) WIP: Added new QGRaphicsItems (FieldOfView, Centroid) Nov 21, 2024
@javierggt javierggt requested a review from Copilot November 21, 2024 18:13
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 suggestion.

@javierggt javierggt requested a review from Copilot November 22, 2024 19:59

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (5)

aperoll/widgets/star_plot.py:180

  • [nitpick] The method name get_moving is ambiguous. It should be renamed to is_moving.
def get_moving(self):

aperoll/widgets/star_plot.py:255

  • [nitpick] The variable name incl_action is ambiguous. It should be renamed to include_acq_action.
incl_action = QtW.QAction("include acq", menu, checkable=True)

aperoll/widgets/star_plot.py:259

  • [nitpick] The variable name excl_action is ambiguous. It should be renamed to exclude_acq_action.
excl_action = QtW.QAction("exclude acq", menu, checkable=True)

aperoll/widgets/star_plot.py:309

  • The hardcoded string 'include slot' should be replaced with a constant or enum for better maintainability.
if centroid is not None and result.text().startswith("include slot"):

aperoll/widgets/star_plot.py:320

  • The hardcoded string 'Show FOV' should be replaced with a constant or enum for better maintainability.
elif result.text() == "Show FOV":
@javierggt javierggt changed the title WIP: Added new QGRaphicsItems (FieldOfView, Centroid) Added new QGRaphicsItems (FieldOfView, Centroid) Dec 2, 2024
@javierggt javierggt requested a review from Copilot December 2, 2024 15:26

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no suggestions.

| (row > 511)
| (col < -511)
| (col > 511)
| (self._centroids["IMGFID"])
Copy link

Choose a reason for hiding this comment

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

Why are the fid light centroids excluded here? They are a bit less interesting, but if we're just trying to display where on the CCD stuff is happening it might make sense to just include them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will add them back. I will use the same symbol but in red.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanconn I just pushed this change in this branch (it was in a branch in the other PR). I am going to merge this PR. I have used this extensively and found no issues, and it is getting harder to keep track of things.

Choose a reason for hiding this comment

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

If you want me to review again let me know.

@javierggt javierggt merged commit 326dbfd into main Jan 28, 2025
2 checks passed
@javierggt javierggt deleted the new-items branch January 28, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants