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

Fix or remove non-SciPy voigt profile code #137

Closed
chummels opened this issue Jun 16, 2020 · 3 comments
Closed

Fix or remove non-SciPy voigt profile code #137

chummels opened this issue Jun 16, 2020 · 3 comments

Comments

@chummels
Copy link
Collaborator

When Trident was first implemented, we calculated the Voigt Profile using code taken from an IDL routine following Clenshaw's Algorithm involving Chebyshev polynomials. However, there was a much simpler approximation that could be made using the SciPy library, which is what is described in our method paper.

Right now, when you attempt to generate a spectrum, every time that you deposit an absorption line as a voigt profile, it examines if you have SciPy installed or not. If you do have it installed, it uses the SciPy manner of generating the voigt_profile (voigt_scipy in absorption_spectrum/absorption_line.py), but if you don't have it installed, it uses the Clenshaw algorithm (voigt_old in absorption_spectrum/absorption_line.py). Recently, we became aware that voigt_old is no longer functioning correctly, in that it is calculating tau values about 10^20 times too high, leaving the user with no flux transmitted. I'm not sure how long this has been an issue, because virtually everyone has SciPy installed (including our testing interface on Travis and CircleCI).

We need to either fix the voigt_old code to correct the problem, or get rid of it entirely and use SciPy as a hard dependency. I don't really like having such a large dependency as SciPy, but on the other hand, having two disparate ways of calculating the voigt profile seems like it's going to open up precision issues for users with different setups. I'm leaning towards just getting rid of the voigt_old code and going SciPy all the way.

@chummels
Copy link
Collaborator Author

Please feel free to add your opinions here.

@brittonsmith
Copy link
Collaborator

+1 for removal. We don't have the person power to fix/maintain it. If a number of people start popping up with scipy installation issues, then we can always revisit this.

@chummels chummels closed this as completed Dec 9, 2020
@chummels
Copy link
Collaborator Author

chummels commented Dec 9, 2020

Addressed by PR #139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants