Skip to content
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

Spline model #804

Merged
merged 14 commits into from
Sep 16, 2022
Merged

Spline model #804

merged 14 commits into from
Sep 16, 2022

Conversation

newville
Copy link
Member

Description

This adds a SplineModel(), which is slightly unusual in that it is variadic, with a variable number of parameters at x values determined by xknots.

Type of Changes
  • Bug fix
  • New feature
  • Refactoring / maintenance
  • Documentation / examples
Tested on

Python: 3.9.13 | packaged by conda-forge | (main, May 27 2022, 17:00:52)
[Clang 13.0.1 ]

lmfit: 1.0.3.post42+g6bea572.d20220830, scipy: 1.9.0, numpy: 1.23.2, asteval: 0.9.27, uncertainties: 3.1.7

Verification

Have you

  • included docstrings that follow PEP 257?
  • referenced existing Issue and/or provided relevant link to mailing list?
  • verified that existing tests pass locally?
  • verified that the documentation builds locally?
  • squashed/minimized your commits and written descriptive commit messages?
  • added or updated existing tests to cover the changes?
  • updated the documentation and/or added an entry to the release notes (doc/whatsnew.rst)?
  • added an example?

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #804 (a56e0d0) into master (6bea572) will decrease coverage by 0.17%.
The diff coverage is 84.78%.

@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
- Coverage   93.80%   93.62%   -0.18%     
==========================================
  Files          10       10              
  Lines        3404     3546     +142     
==========================================
+ Hits         3193     3320     +127     
- Misses        211      226      +15     
Impacted Files Coverage Δ
lmfit/models.py 91.07% <84.78%> (-0.53%) ⬇️
lmfit/model.py 90.95% <0.00%> (-0.13%) ⬇️
lmfit/lineshapes.py 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rayosborn
Copy link
Contributor

Is there any reason not to add it to the lmfit_models list? Also, the docstring for SplineModel says the maximum number of knots is 100, whereas MAX_KNOTS is set to 300.

I wondered if there was a way to shoehorn valid_forms into the model, as was done with degree in PolynomialModel, but it's probably not possible, since xknots can have arbitrary values. That's not really a problem though.

Copy link
Contributor

@reneeotten reneeotten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @newville this looks like a useful model to add! I notice there are some trivialities with the documentation and code-style; I know you are not a fan of these... so I am happy to fix those for you. I'll add a commit shortly and then take another look at the actual content!

return log(loren) * gradient(gauss) / gradient(x)
loren = lorentzian(x, amplitude=amp, center=cen, sigma=sig)
gauss = gaussian(x, amplitude=amp, center=cen, sigma=sig)
return log(loren) * gradient(gauss) / gradient(x)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure this will render correctly indented Python code on the documentation website (here and in other places where you made similar whitespace changes) so that, for example, people can copy-and-paste code from the docs to their Python shell?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what is happening there... My editor seems to be putting tabs into rst files for indents. They do seem to render to HTML correctly

doc/builtin_models.rst Outdated Show resolved Hide resolved
@newville
Copy link
Member Author

@rayosborn Thanks -- added Spline to lmfit_models and uniformly set the maximum number of knots to 300.

@newville
Copy link
Member Author

@reneeotten It appears to me that "codespell" cannot be used in a "warn only" mode. For lmfit/models.py it said that having a variable named mone was an error. I am not changing the code to satisfy a spell checker. I am not maintaining a list of words that codespell falsely identifies as misspellings of other words.

I get that spell-checking code is useful. It really cannot prevent merges and trying to fight against falsely identified problems is just of waste of our time.

@reneeotten
Copy link
Contributor

@newville I've fixed the pre-commit issues already; if you're not actively working on it now; I'll push the changes shortly - okay?

@newville
Copy link
Member Author

@reneeotten yep, go for it!

@reneeotten
Copy link
Contributor

this commit should fix the last pre-commit failure... I'll take a closer look at the actual content later in the week

@newville
Copy link
Member Author

@reneeotten OK, thanks!
I'm kind of reluctant to have to maintain lists of code variables that are falsely identified as misspellings. I would be very much in favor of a warning here.

@newville
Copy link
Member Author

newville commented Sep 8, 2022

@reneeotten OK to (squash and) merge?

@reneeotten
Copy link
Contributor

@reneeotten OK to (squash and) merge?

Yes, looks good to me!

@newville
Copy link
Member Author

@reneeotten OK, merging this too.

@newville newville merged commit 3bef74f into master Sep 16, 2022
@reneeotten reneeotten deleted the spline_model branch September 17, 2022 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants