Skip to content
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

BUG: Fix parse_spec #103

Merged
merged 3 commits into from
Jan 29, 2020
Merged

BUG: Fix parse_spec #103

merged 3 commits into from
Jan 29, 2020

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jan 27, 2020

Fix #102

Address part of spacetelescope/synphot_refactor#237

TODO

  • Add tests to actually test the integrated flux (?) of parse_spec results and compare to doing it the non-IRAF way.

@pllim pllim added this to the 0.2.2 milestone Jan 27, 2020
@pllim pllim requested a review from ibusko January 28, 2020 22:04
TST: Pin synphot too in older versions job.
for arg in args:
try:
names.append(arg.meta['expr'])
except Exception:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't be safer to look for specific types of Exception instead of the generic base type? In the way it's here, the caller wouldn't ever be able to see errors that may happen inside the try-except block. One case that we can easily spot is a KeyError from the meta['expr'] construct.

Copy link
Collaborator

@ibusko ibusko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to my comment on generic Exception handling, it would be good to add a couple of test cases that actually result in an exception being raised by the parser. That way we would ensure that the code is handling things in the desired way.

@pllim
Copy link
Contributor Author

pllim commented Jan 29, 2020

@ibusko , good point on the blind exception handling. I replaced it with a more explicit if-else.

'rn(crcalspec$bd_75d325_stis_002.fits, band(u), 9.5, vegamag) * '
'band(fos, blue, 4.3, g160l)')
# NOTE: No expr for this combo.
_single_functioncall(sp1, SourceSpectrum, None, '')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibusko , as discussed in spacetelescope/synphot_refactor#237, this is the only string among those being tested that does not have a "name."

@pllim
Copy link
Contributor Author

pllim commented Jan 29, 2020

Re: extra tests -- I cannot think of any use case that would result in sp.meta['expr'] = '()' (the Exception part that is now an else). If you have one for me to add to the tests, I would be happy to.

@pllim pllim merged commit e55fb0d into spacetelescope:master Jan 29, 2020
@pllim pllim deleted the parse-rn-name branch January 29, 2020 17:58
pllim added a commit that referenced this pull request Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: parse_spec for "em" might be wrong if you use synphot>=0.1.2
2 participants