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

Made change to address http://projects.scipy.org/scipy/ticket/1531 #92

Closed
wants to merge 4 commits into from

Conversation

calbaker
Copy link
Contributor

Now, if p0 is given and is scalar, it is recast as a numpy array with a size of 1.

Now, if p0 is given and is scalar, it is recast as a numpy array with a size of 1.
@rgommers
Copy link
Member

The behavior for p0 scalar is not documented, and it will change with this patch. I like your change though, it's more robust and less magical (now you can specify p0 = 3 and the initial guess will be [3, 3] for a function that takes 2 parameters).

The bug you're seeing though, is because self is counted as input parameter for a method. This is still broken for p0=None. The solution is to check if 'self' is the first element of args.

@rgommers
Copy link
Member

Could you please also create a regression test for your fix?

@calbaker
Copy link
Contributor Author

I am fairly new to this, and I honestly have no idea what a regression test is. Is there is a good web resource that can explain it?

I'll try to write something up that checks if 'self' is the first element of args.

@pv
Copy link
Member

pv commented Oct 31, 2011

Hi, regression test is just a test that checks that the bug is fixed (so that it doesn't get reintroduced by accident).

@calbaker
Copy link
Contributor Author

So do you mean that I need to create code that will fail if the bug gets reintroduced but continue to run correctly if the bug remains fixed?

@rgommers
Copy link
Member

For the test, have a look at test_minpack.py in scipy/optimize/tests/. There's a class TestCurveFit to which you can add a new test. The test should fail without your fix, pass with it, and be as simple as possible.

Perhaps the term "unit test" is more familiar to you. If not, it's worth learning about. There are tons of resources, google is your friend.

The numpy/scipy testing guidelines are at https://github.com/numpy/numpy/blob/master/doc/TESTS.rst.txt

@rgommers
Copy link
Member

Exactly, that's the point. Making sure that once it's fixed it stays fixed.

@calbaker
Copy link
Contributor Author

Ok, I will do all that then. I'm a mechanical engineer so I'm barely familiar with computer science terminology. I've used SciPy enough that I feel obligated to make it better when I encounter a problem that I understand well enough to fix.

@rgommers
Copy link
Member

Great, we appreciate it. If you run into problems let us know.

By the way, if you add commits to your own git branch that you sent the pull request from they automatically show up here.

@calbaker
Copy link
Contributor Author

Ralph, how can I check if self is the first element in the arguments of args?

@calbaker
Copy link
Contributor Author

Ralph, please take a look at my most recent commit. It's not handling the if statement that checks for 'self' in args properly. I'm not exactly sure how to fix this, but I think this is a good start toward getting the test_minpack.py and minpack.py working correctly.

@rgommers
Copy link
Member

Hi, your check for 'self' was fine, the only problem was that the method in your test was defined incorrectly. It should have had self as first parameter. I've fixed this and merged your commits at rgommers@9f69cfcb1b82e. That looks fine to go in now. Thanks!

@rgommers
Copy link
Member

Pauli, can you please have a look at the change in behavior for scalar p0? Before it was possible to give a scalar initial guess for a function that takes multiple input parameters, and the guess would be expanded from p0=2.1 to for example p0=[2.1, 2.1, 2.1] if the function takes 3 params.

This behavior was undocumented, and imho undesired. It raises an error now.

@josef-pkt
Copy link
Member

FWIW: I never realized this undocumented behavior for a scalar p0, and I'm in favor of the change.
as reference http://stackoverflow.com/questions/7615733/too-many-arguments-used-by-python-scipy-optimize-curve-fit is where the discussion started

@rgommers
Copy link
Member

OK, thanks Josef. Pushed.

@rgommers rgommers closed this Dec 20, 2011
@calbaker
Copy link
Contributor Author

calbaker commented Jan 3, 2012

So this means my changes will go into the next release of SciPy, right? If so, yay, I'm glad I could contribute!

@rgommers
Copy link
Member

rgommers commented Jan 3, 2012

Yes, it will be in SciPy 0.11.0, and you'll be listed as a contributing author in the release notes.

Thanks again, and hope this won't be your last contribution:)

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.

None yet

4 participants