-
Notifications
You must be signed in to change notification settings - Fork 107
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
Script that allows interactive editing of the slit/order edges #1713
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1713 +/- ##
===========================================
- Coverage 40.98% 40.74% -0.24%
===========================================
Files 190 193 +3
Lines 43904 44180 +276
===========================================
+ Hits 17994 18003 +9
- Misses 25910 26177 +267 ☔ View full report in Codecov by Sentry. |
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 is really impressive @kbwestfall! The structure of the GUI is really clear, and much better organised than what I previously implemented in identify
, skysub_regions
, and object_find
. Eventually, we should aim to migrate these over to the same structure that you've setup to make all GUI codes a bit easier to manage in the long run.
I have mostly minor comments, but I'm a little confused why there are some unused arguments being passed to some functions (including a few *args
and **kwargs
as well). In some cases, there doesn't appear to be any inheritance, and I can't understand why they are needed... I'm certain that I'm about to learn something new in python that I didn't know before!
Without major comments, I'm approving - thanks again! 👍
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.
Now actually approving...
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.
Thanks @rcooke-ast ! I've written a few of these now and stackoverflow is key!
On the argument lists, as you'll see in my responses, some of these were pure laziness and others were restrictions to the required calling sequence for cursor events.
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.
Solid! Thanks @kbwestfall!
As titled. See changes to
doc/calibrations/slit_tracing.rst
for a description of the script.I'm directing to the
hires_trace_orders
branch because I branched from there. Will redirect once that PR clears.I should add some tests, and it would be useful for one or more volunteers to take this for a test drive.