search

shihyuntang / igrins_rv

Welcome to IGRINS RV, a open source pipeline for extracting radial velocity (RV) for the Immersion GRating INfrared Spectrometer (IGRINS) instrument. Please contact asa.stahl@rice.edu or sytang@lowell.edu for more detail.
https://igrins-rv.readthedocs.io
MIT License
6 stars 1 forks source link

TelFit Dependency Strategy #3

Closed gully closed 3 years ago

gully commented 3 years ago

Tradeoff of TelFit fork and future-proofing

As (bad?) luck would have it, the fortranformat dependency of TelFit was updated two days ago for the first time in seven years. This update introduced backwards-breaking changes and made the TelFit installation error during the python setup.py install phase. I reported the issue in https://github.com/kgullikson88/Telluric-Fitter/issues/25 and made a pull request https://github.com/kgullikson88/Telluric-Fitter/pull/26.
The igrins_rv choice to include raw TelFit source code directly as part of this repository has some benefits and drawbacks. On one hand you can control the exact behavior of TelFit and modify its code to work more closely with igrins_rv: deleterious changes to telfit are kept out. On the other hand, you have to maintain your fork and do not effortlessly benefit from TelFit maintenance: positive changes to TelFit are kept out. In this example, users of igrins_rv attempting to follow your installation instructions would encounter this fortranformat problem and not automatically benefit from the upstream fixes.
Changes to TelFit are small and infrequent at the moment---averaging 3 Pull Requests in ~4 years---so it is easy enough to make those changes manually. The main drawback I see with the forking of TelFit is to close off the development ecosystem. A forked TelFit makes it ever-so-slightly harder for prospective developers of igrins_rv to interoperate: if I have a separate code that depends on the original TelFit, I cannot simultaneously use the two.

The current change to TelFit is a single line of code: changing the Default wavelength scaling from air to vacuum. Is there a way to change this default in the execution call to TelFit rather than forking the master version?

TelFit Installation woes act as a barrier to entry

Another TelFit-related thorn is that TelFit is hard to install. The TelFit Issues have several instances of folks reporting installation woes. Anecdotally, several researchers have personally asked or emailed me for help to install, or have reported abandoning altogether after TelFit installation failed. I'm a contributor to TelFit and have a working version on my computer and still had a hard time reinstalling this version for igrins_rv. I'm not sure what the solution is, since again that's more of an "upstream problem", but it does have consequences for igrins_rv--- TelFit currently acts as a barrier to entry for new users (especially students) and may limit the reach of the package. In essence, what steps can be made to make it easier to install TelFit?

shihyuntang commented 3 years ago

Hi @gully ,

Thank you for bring this up! The one line change in the Telfit source code is not a bug, but more like missing features(?). I just open an issue on Telfit kgullikson88/Telluric-Fitter#27 asking for "adding the option to change the status of vac2air from the call of the TelluricFitter class." After this feature been added in Telfit main repo, modification on IGRINS RV is straightforward.

shihyuntang commented 3 years ago

Updates: With the pull request (kgullikson88/Telluric-Fitter#30) on Telfit been merged, IGRINS RV is now no longer need to pack with the Telfit code.

As for the TelFit Installation woes act as a barrier to entry. Without modifying the Telfit code, the best we can do now is to make the installation instruction from source as detailed as possible. I added in the FAQ for How to install the right gfortran version for Telfit?.

I would love to help you and Kevin on making Telfit pip installable! In that case, all I need to do is add Telfit to .yml list.

shihyuntang commented 3 years ago

Hi @gully ,

I made a pull request on Telfit that fix the error while compiling the LBLRTM with gfortran (see this issue on Telfit for more details). If this fix can be verified by you or Kevin, making Telfit pip installable with py3 will help a lot for IGRINS RV --- overcome the TelFit Installation woes act as a barrier to entry problem