Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[bump minor] Add new FVoigt script #1037
base: master
Are you sure you want to change the base?
[bump minor] Add new FVoigt script #1037
Changes from 2 commits
0d3902b
d786ea7
c40f548
4f43b13
0aebbda
825fd30
d28c643
c0241dc
bab22fb
1acbad1
e963947
e938ad5
eb47ce3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 mergeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...