-
Notifications
You must be signed in to change notification settings - Fork 273
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
Add Pearson4 fit model (original Wikipedia version) #800
Conversation
@lellid Thanks! This looks like a very good start to me, and I'll try to take a more careful look at it. FWIW, I didn't understand the failures (files were modified by |
I can, the changes/additions to import packages do not follow the guidelines of the pre-commit hooks we use. That's something that could be verified locally before submitting a PR, but in short the diff for
|
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.
thanks for the PR @lellid - a few small comments after a quick look through
@reneeotten Ah, thanks -- "imports out of preferred order"! |
well... the main thing is you're not supposed to have several "import ..... as ....." on one line as was done in this PR: |
- add tests - improve documentation - fix flake8 complainings
Hi, I just updated this PR. I
|
@lellid @reneeotten if I understand correctly, we're all in favor of merging this, except for some "not-quite-as-beautiful-as-we-like" |
I used iSort to sort the using statements, so now it passes the tests. |
@lellid Thanks! I'll plan to merge in a couple of days to give time for final comments or suggestions. |
thanks @lellid - for me the two comments about the docstrings (I left them again above) remain, after making these changes - or explaining why I'm wrong ;) - the commits should be squashed into one - then it's ready-to-go! |
@reneeotten @lellid Yes, let's please have consistent code and docstrings! I think we are happy to merge this, but we kind of need a version we can support, which does include being able to explain the actual definition used. We discussed this earlier at #799, but this PR was created as a separate PR after that discussion. To reiterate, I strongly suggest following the Wikipedia definition or having a really clearly documented explanation of why that form was used instead of the Wikipedia definition. |
Hi, I consolidated the duplication of the docstring. To the other question about the Wikipedia definition and my implementation: It is simply a numerical thing: Of course I can use a naive implementation of So I logarithmize everything, and put all terms then in the I have seen other rather naive implementations in your code (for instance, Pearson7). Maybe, in the future, I will make some pull requests to correct that. |
thanks!
That might be clear to you since you've probably stared at that equation for a while and looked at the math, but from a first read it wasn't to me. I also haven't checked that this is all implemented correctly, but I assume that you did... Of course all of this could be considered a "me problem" but perhaps you could just add a note to the function in
Well, naive or not it appears to have been working well for people for quite some time - of course improvements are always welcome. Thanks again @lellid - this PR should be almost at the finish line. |
Hi, what I was really wondering about was why you don't have any tests for the function values in your code? At least I haven't seen any. Would make those numerical tweakings a lot more secure, if you can be sure that the values come out that you expect. |
OK, thanks. Well, at some point we might fix the capital "I" to "j" (Python's preferred sqrt(-1)).
These functions are kind of prone to under/overflow. I would not necessarily agree that it is obvious that taking
Do you mean to use But, I'm not sure that the general approach of "work in log space and then take exponential" is necessarily worth the complication. Readability definitely counts. |
Probably because we feel like we don't need those specifically. The tests of fitting with these functions do test the values of parameters, so if the functions break, that will very likely be reflected in the fit results.
Well, I guess we expect that we are not changing the lineshape definitions very much ;) |
@reneeotten @lellid I'm just going to squash and merge this. If there are more issues to resolve, they can be done afterward. |
Description
This adds the Pearson4 fit model (original version from Wikipedia), which is a skewed version of Pearson7.
Type of Changes
Tested on
Python: 3.10.4 | packaged by conda-forge | (main, Mar 30 2022, 08:38:02) [MSC v.1916 64 bit (AMD64)]
lmfit: 0.0.post2625+g0b9947a.d20220808, scipy: 1.9.0, numpy: 1.23.1, asteval: 0.9.27, uncertainties: 3.1.7
Verification
Have you