Fix mixed unit error between angstroms and wavenumbers in extinction_extension.py#864
Conversation
|
There is an error that causes the code not to run. Needs a fix of course. |
|
@karllark, I've tried a couple of different approaches to address the git test errors. Since I can't replicate them in my local runs, I'm out of ideas—unless I modify the testing code inside the external Just in case, my local python version is 3.12.3, Astropy version is 7.2, and Numpy version is 2.4.6. The different dependency versions might be a culprit. |
|
I wonder if you have an old version of dust_extinction installed. Maybe try updating it and seeing if that changes things. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #864 +/- ##
==========================================
+ Coverage 39.44% 39.48% +0.03%
==========================================
Files 110 110
Lines 10756 10763 +7
==========================================
+ Hits 4243 4250 +7
Misses 6513 6513 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if x.unit == u.micron**-1: | ||
| x_um_inv = x | ||
| elif x.unit == u.Angstrom: | ||
| x_um_inv = x.to(u.micron**-1, equivalencies=u.spectral()) |
There was a problem hiding this comment.
I think this can be just one line. Line 95 should give both behaviors. If already in 1/micron, it will do nothing.
karllark
left a comment
karllark
left a comment
There was a problem hiding this comment.
Looks great. One minor comment that may reduce the number of lines of code.
| if x.unit == u.micron**-1: | ||
| x_um_inv = x | ||
| elif x.unit == u.Angstrom: | ||
| x_um_inv = x.to(u.micron**-1, equivalencies=u.spectral()) |
karllark
left a comment
karllark
left a comment
There was a problem hiding this comment.
One last spelling error to fix than then I'll merge.
| or assumes wavelengths in wavenumbers [1/micron] | ||
|
|
||
| internally wavenumbers are used | ||
| internally wavenubmers in inverse micron are used |
There was a problem hiding this comment.
Argh. Spelling. Should be "wavenumbers."
2 participants
The bug was caused by two main issues within the
extinction_extension.py: (1) a mixed usage of angstroms and wavenumbers, and (2) inconsistent input formatting requirements, where certain steps expected raw numerical values while others required values paired with Astropy units. I am opening this PR to resolve these mismatches. This PR addresses the issue #863.