-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
- 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
There was a problem hiding this 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.
There was a problem hiding this 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 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":
There was a problem hiding this 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 no suggestions.
aperoll/star_field_items.py
Outdated
| (row > 511) | ||
| (col < -511) | ||
| (col > 511) | ||
| (self._centroids["IMGFID"]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
The purpose of this PR is to:
This PR makes the following changes:
star_field_position
that is used by all items to recalculate their position when the scene attitude changes.StarField
behavior into a dataclass of typeStarFieldState
. The idea is that this is the one place to find all run-time switches that the user can change.StarView
soStarView.catalog
is neverNone
, and addsCatalog.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 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
Independent check of unit tests by [REVIEWER NAME]
Functional tests
The usual aperoll command to inspect catalogs
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.