-
Notifications
You must be signed in to change notification settings - Fork 76
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
Aperture validate after selection #2684
Aperture validate after selection #2684
Conversation
9d5faa3
to
d7f3c70
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2684 +/- ##
==========================================
- Coverage 90.91% 90.89% -0.02%
==========================================
Files 163 163
Lines 21348 21387 +39
==========================================
+ Hits 19408 19440 +32
- Misses 1940 1947 +7 ☔ View full report in Codecov by Sentry. |
if aperture is not None or dataset is not None: | ||
reg = self.aperture._get_spatial_region(subset=aperture if aperture is not None else self.aperture.selected, # noqa | ||
dataset=dataset if dataset is not None else self.dataset.selected) # noqa | ||
# determine if a valid aperture (since selected_validity only applies to selected entry) | ||
_, _, validity = self.aperture._get_mark_coords_and_validate(selected=aperture) |
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.
I cannot tell which is better. But we can also just grab the 3rd result and not bother with magic underscores.
_, _, validity = self.aperture._get_mark_coords_and_validate(selected=aperture) | |
validity = self.aperture._get_mark_coords_and_validate(selected=aperture)[2] |
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.
I find the underscores to be more readable myself 🤷♂️
This comment was marked as outdated.
This comment was marked as outdated.
* currently via property, but probably want this hooked into a traitlet so that the plugin can observe that and show message, etc
ee06116
to
acd2d37
Compare
# if self.scale_factor == 1.0: # this would catch annulus, which might cause confusion | ||
# pass |
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.
Can this be removed?
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.
I could rewrite it as a comment, but do think we want something here so that we don't accidentally reintroduce it (since it feels like this should be there)
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.
Changing the subset leads to the results you detail in the description, so LGTM, thanks!
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.
Can the snackbar message that is triggered when you try to select a composite subset be avoided?
'Failed to extract Subset 1: AttributeError("'OrState' object has no attribute 'roi'")'
Not easily since its hard to predict the order that the observes will be evaluated. We could refactor the logic, but I also think the extra warning that an input is unsupported doesn't hurt? |
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.
This also works as intended for me, approving
Description
This pull request allows non-apertures as choices in subset dropdowns for aperture photometry and spectral extraction and validates them during selection (or on a change to the underlying subset) with the following behavior:
TODO:
Screen.Recording.2024-01-31.at.1.43.40.PM.mov
Screen.Recording.2024-01-31.at.1.52.54.PM.mov
Screen.Recording.2024-01-31.at.2.09.39.PM.mov
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.