-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add widget to lt_xspec for selecting between multiple input spectra #403
Conversation
May I review this? |
Please do! |
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 left some initial coments. I haven't testet the GUI yet... do you have a file with multiple spectra on it for testing?
linetools/guis/spec_widgets.py
Outdated
dline.analy['spec'] = tspec | ||
dline.limits.set(iwv) | ||
dline.measure_ew() | ||
mssg = 'Using dummy '+ dline.__repr__() |
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.
change message to "Assuming line is "
(?)
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.
These two lines seem duplicates from 471 and 472. May be best to take them out of the if
and else
statements.
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 so a user can measure the EW without setting a line.
Have made a small edit.
I tested the GUI and everything seems good. I'm not so convinced whether we want to start labeling the spectra from I also left comments on the code while ago (see above)... not sure if you considered them. |
if hasattr(spec, 'labels'): | ||
spec_labels = spec.labels | ||
else: | ||
spec_labels = ['{:d}'.format(ii) for ii in range(spec.nspec)] |
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.
{:d}'.format(ii)
-> {:d}'.format(ii+1)
(?)
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 think I prefer Python indexing. That is the way
the spectra are truly indexed anyhow.
Merging |
As titled.
Also adds functionality to dynaimcally set gui size
based on number of pixels in the monitor.
No new docs or tests.