search

kgullikson88 / Telluric-Fitter

Telluric fitting made easy
http://telfit.readthedocs.org/en/latest/
MIT License
20 stars 17 forks source link

Python 3 Compatibility #21

Closed astahl7 closed 3 years ago

astahl7 commented 4 years ago

Hi Kevin,

I've found your telluric fitter code very useful and would like to incorporate it into a pipeline I'm writing to estimate radial velocities from infrared spectra. However, I've noticed you've written that the package is only compatible with python 2.7. As python 2.7 is officially defunct now, I was wondering what dependencies (if any) Telfit uses that are incompatible with python 3. Or is it just a matter of digging through the code and fixing all the print statements?

Thank you for your time and help, Asa

PS - Sorry to post this as an issue, but I couldn't find any other way to contact you.

gully commented 4 years ago

Hey @astahl7 I can ask Kevin about this the next time I see him, but I'm also tentatively interested in migrating telfit to Python 3, as well as exploring prospects for using telfit in an inference procedure such as Enhancement proposal #11. I suspect it'd be a while until these changes got merged into a user-friendly package, but experimental/development versions could be feasible, so consider subscribing to this or other related Issues for updates. Thanks for your interest in TelFit.

Updated to add: I'm happy to help review a pull request or consult on a roadmap if you are open to trying to migrate the code.

astahl7 commented 4 years ago

Hi @gully,

A colleague of mine has actually been working on migrating it, and I believe has gotten the package to function normally (apparently, there wasn't too much that needed modifying). I'll speak with him about getting in touch with you.

One other question, while I have you: how accurate is the wavelength calibration of Telfit's internal model? For instance, I have an observed telluric spectrum whose wavelength calibration is slightly off. I can fit it using an observationally derived telluric template, such as https://www.nso.edu/data/historical-data/the-solar-flux-spectrum/, and obtain a solid wavelength solution. But on nights with strong telluric absorption the template is less than ideal. Can I use Telfit with adj_wave = data and expect the model it is calibrating the data against to be precise to, say, 1 m/s? Or am I better off doing a sort of two-step process, first fitting the observations with the template, using that to get a decent wavelength solution, and then applying Telfit with adj_wave = model?

gully commented 4 years ago

Great, glad to hear the Python 3 migration is underway! If you and your colleague are open to it, I'm sure it'd be welcomed as a pull request, and I can help consult on how to do that if you're open to it.

Your question on the radial velocity precision sounds like a topic of a worthy research project. Here is an article that may be helpful, see subsection "wavelength calibration as added value", and references therein.

astahl7 commented 4 years ago

Interesting! Thanks for the link. It seems like the HITRANS line database can pretty variable in precision (m/s to 100s of m/s), so I will probably have Telfit use my own wavelength calibration.

Which brings up one last thing: what is the purpose of the 'wavestart' and 'waveend' parameters fit by Telfit? What does it mean to let them vary when adj_wave = 'model'? The reason I ask this is because while the documentation says that a fitter instance's output "model.x" should match the input "data.x" such that the two are directly dividable, a side-by-side comparison shows this is not the case.

gully commented 4 years ago

I don't know the answer to your question! Good luck experimenting!

awmann commented 3 years ago

Greetings from the distant future. What is the status here? Is there a Python3 version of this code somewhere?

astahl7 commented 3 years ago

The version of Telfit available here is compatible with python3 - I've been using it just fine. The bug noted in the Issues regarding adding the extra space in some of the fortran code that comes with the package still seems to be present, though.

PS - Big fan of your work!

gully commented 3 years ago

Yeah, #22 made it work for me on python 3.7, though I only use a subset of the capabilities.

jradavenport commented 3 years ago

Hello from 2021, has Py >3.7 been tested much? I'd love to use Telfit in a reduction pipeline we're putting together for a new longslit spectrograph... and even better would love to see it possibly included in Astropy's Specreduce

gully commented 3 years ago

Hello all, the code has been python compatible for 1 year :partying_face: Thank you to @shihyuntang for doing the most extensive testing with Python 3. We have also just accepted @shihyuntang 's pull request #32 which will momentarily/hopefully allow the code to become pip install-able (barring any packaging woes). I have tagged a new release (v1.4). As of v1.4 we are dropping Python 2 support. Python 3 support is also limited since there is currently no financial support for TelFit. We are jazzed to see an uptick in community contributions and encourage many more Pull Requests! We would especially value an financial contributions and in-kind contributions to modernize the codebase.

@jradavenport please report back any testing analysis, patches, or architectural design overhauls as a PR or GitHub Issue if you encounter any friction while using TelFit for your new longslit spectrograph.

I am closing this issue since the code is indeed :heavy_check_mark: Python 3 Compatible. :smiley: