[bump minor] Add new FVoigt script #1037
Conversation
bin/picca_compute_fvoigt_hcd.py
Outdated
|
|
||
|
|
||
| def voigt_profile(x, sigma=1, gamma=1): | ||
| return np.real(wofz((x + 1j*gamma) / (sigma * np.sqrt(2)))) |
There was a problem hiding this comment.
is there a reason to define the voigt profile function here instead of just using scipi.special.voigt_profile in the first place?
There was a problem hiding this comment.
I just did a quick test, and the two functions don't produce the same results. In fact our version appears to be off by exactly a factor of sqrt(pi) from the scipy version. @moonlovist are you sure we got the correct function here?
There was a problem hiding this comment.
I think the difference might be in using the Voigt function V in case of scipy and H in case of this computation, defined like this on wikipedia
.
Anyway, using wofz or voigt_profile straight from scipy is fine (with the correct normalizations), just improves readability as you don't need to redefine things here, scipy is using wofz internally anyway (but in a c-based routine).
There was a problem hiding this comment.
If both are the same (once properly normalized), I would suggest changing this to using scipi.special.voigt_profile given that this is what we use in the DLA masking module, but this would not be a show stopper for the merge
There was a problem hiding this comment.
they are as the scipy.special.voigt_profile is just a cythonized wrapper around woffz anyway, I'd mildly prefer using existing functions (with some comment about the difference in intrinsic normalization) over having our own version for smaller things like this, even more given we're using the scipy function elsewhere anyway...
|
also: please add docstrings and ideally a test... |
|
is this ready for review? |
|
Yes, this has been tested and working for a while now. |
bin/picca_compute_fvoigt_hcd.py
Outdated
|
|
||
|
|
||
| def voigt_profile(x, sigma=1, gamma=1): | ||
| return np.real(wofz((x + 1j*gamma) / (sigma * np.sqrt(2)))) |
There was a problem hiding this comment.
If both are the same (once properly normalized), I would suggest changing this to using scipi.special.voigt_profile given that this is what we use in the DLA masking module, but this would not be a show stopper for the merge
|
Hi all, I modified the file so that it now uses functions already defined in dla_mask, instead of defining new functions that do the same things. @andreicuceu can you check that I did not accidentally add a normalisation issue? |
|
I fixed the typo and added the lyman beta absorption, but tests are still missing |
|
tests are failing due to a weird coveralls error. Maybe their server is down. Otherwise, they might have updated something and the new version is not backwards compatible. We should try to rerun failing tests in a while to see if it was just their server being down or not |
|
somewhere things are going wrong, here: |
|
What is the status of this PR, @iprafols , @Waelthus , @andreicuceu ? |
|
We briefly discussed this PR in the latest Lya software telecon. If I remember correctly Ting mentioned that he wanted to work on that, I am not seeing him in the picca package. @andreicuceu any news on this PR ? |
|
I personally last looked at this with @moonlovist (i.e. Ting) in Hawaii. Not sure what the current status is. But given that the PR just adds a new script, there shouldn't be any issues outside of this new routine when merging later. |
|
Ok, sorry @moonlovist I did not recognize your github name. |
merging master
A simplified version of the FVoigt computation, based on the code by @moonlovist. Work in progress.