-
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
Transition lookup by Zion tuple #422
Conversation
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.
Code looks good.
Need to add a test
And update docs (not doc string)
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.
See my comment regarding duplication of code. I also think it would be good to have a test added and update the documentation (.rst) accordingly.
linetools/lists/linelist.py
Outdated
@@ -595,6 +597,9 @@ def all_transitions(self, line, debug=False): | |||
elif isinstance(line, Quantity): # Rest wavelength (with units) | |||
data = self.__getitem__(line) | |||
return self.all_transitions(data['name']) | |||
elif isinstance(line, tuple): # Zion input | |||
data = self.__getitem__(line) | |||
return self.all_transitions(data['name'][0].split(' ')[0]) |
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 seems duplication of code. You could simply replace current line 597:
elif isinstance(line, Quantity):
-> elif isinstance(line, (tuple, Quantity)):
.
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.
Fair enough, but then I still have test whether 'line' is a tuple because of the differences between lines 599 and 602.
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.
The spliting of name should be done in current line 596...
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.
Only if all_transitions returns just 1 transition...
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.
Oh, I see. I would prefer avoid duplication of code, so maybe the solution is to have the code check whether the output is dict
(implying 1 transition) or Table
(implying >1 transition).
Yup
…On Thu, Oct 19, 2017, 14:52 jnburchett ***@***.***> wrote:
We cool, @profxj <https://github.com/profxj> and @ntejos
<https://github.com/ntejos> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#422 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD0BjVhHXso63bnmt8yI70HYAr8EtJhrks5st8SwgaJpZM4P9BVh>
.
|
The other examples around it are MgII, CIII, and HI!!! I'm merging. |
Make LineList.all_transitions and LineList.strongest_transitions accept Zion tuple, e.g., (8,6)