search

kgullikson88 / Telluric-Fitter

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

Passing air_wave by user in GenerateModel #27

Closed shihyuntang closed 3 years ago

shihyuntang commented 3 years ago

Hi,

Our radial velocity pipeline shihyuntang/igrins_rv really benefits from the Telfit code in terms of RV precision.

In our case, after using the TelluricFitter.Fit function to get the best match parameters, we want the TelluricFitter.GenerateModel function to be able to generate a model spectra under nofit=True mode in vacuum space.

Could @gully or @kgullikson88 make the self.air_wave changeable in the TelluricFitter.GenerateModel function? Currently, we fork and include Telfit to our repo and manually change the TelluricFitter.py

line 672: resolution=None, vac2air=self.air_wave)

to 

line 672: resolution=None, vac2air=false)

Adding the option to change the status of vac2air from the call of the TelluricFitter class would help us from not including TelFit in our RV package but simply tell our users to clone from the main Telfit repo. And we would all be able to benefit from Telfit's updates. Thank you!

kgullikson88 commented 3 years ago

Unfortunately, I don't have much time to maintain this project anymore. I'm very glad to hear it is working well for you, and for IGRINS no less! It sounds like you have a fix in a forked repo - could you just make a pull request with that fix?

shihyuntang commented 3 years ago

Hi @kgullikson88 ,

I just made a pull request. Only 3 lines of adding. Thank you!

shihyuntang commented 3 years ago

Hi @kgullikson88 ,

In that pull request, I also updated with option to let user suppress fortran outputs. @gully suggested this during the JOSS review at shihyuntang/igrins_rv#4. However, the suppress_stdout function suggested by @gully do not work on subprocess. Therefore, I come up with a solution by adding stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL to the subprocess.check_call. I hope this do not conflict with any other functions.

Thank you!

gully commented 3 years ago

This Issue was closed in PR #30 Notably this change may be slightly backwards breaking? Existing users who are used to the old behavior may wish to spot-check model output to ensure continuity with previous results? If this scenario describes you:

1). beginner please open a new Issue and report any differences in behavior you see from old version to versions after PR #30. 2). advanced Design and implement unit tests to catch these sorts of breaking changes in the future.

Thank you! :pray: