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

DOC: #904 #967

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

DOC: #904 #967

wants to merge 2 commits into from

Conversation

mstekiel
Copy link

Description

Updating multifit example based on discussion #904

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

Python: 3.9.15 (main, Nov 24 2022, 14:39:17) [MSC v.1916 64 bit (AMD64)]

lmfit: 1.2.2, scipy: 1.9.1, numpy: 1.23.5, asteval: 0.9.29, 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?

@newville
Copy link
Member

@mstekiel Thanks! I think would make two requests:

a) use pre-commit to find some recommended (ok, slightly required) styling changes.
b) make this very nice example be a separate file from the existing version. That is, let's have both a Model-based version and a minimzer-based version.

@reneeotten
Copy link
Contributor

reneeotten commented Aug 21, 2024

b) make this very nice example be a separate file from the existing version. That is, let's have both a Model-based version and a minimzer-based version.

I'd be in favor of keeping it in one example file - both approaches can be shown/compared in one file.

Also please write a meaningful commit message and you can probably close the discussion topic by adding, the following line to the body of the commit message:

Closes: #904

@newville
Copy link
Member

@mstekiel @reneeotten Do you think it would be too long to show both ways in one file? It's kind of long as it is.

The objective function is a little different, even if the numerical results are the same.

Having both ways in one file would be OK, but I don't think we should replace the existing example with one use Model and then having to unpack and recombine that in an objective function.

@reneeotten
Copy link
Contributor

@mstekiel @reneeotten Do you think it would be too long to show both ways in one file? It's kind of long as it is.

The objective function is a little different, even if the numerical results are the same.

Having both ways in one file would be OK, but I don't think we should replace the existing example with one use Model and then having to unpack and recombine that in an objective function.

@newville I see your point; having two files is fine!

This reverts commit fb9280d.
@mstekiel
Copy link
Author

pre-comit doesn't work for me. It has some problems with the ssl library. I tried to solve it, but didn't manage.

The example is there, you may want to make the commit yourselves and close this pull request.

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