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

Closed
wants to merge 4 commits into
from

4 participants

@calbaker

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

@calbaker calbaker Made change to address http://projects.scipy.org/scipy/ticket/1531
Now, if p0 is given and is scalar, it is recast as a numpy array with a size of 1.
78667d9
@rgommers
SciPy 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
SciPy member

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

@calbaker

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
SciPy 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

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
SciPy 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
SciPy member

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

@calbaker

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
SciPy 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

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

@calbaker

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
SciPy 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@9f69cfc. That looks fine to go in now. Thanks!

@rgommers
SciPy 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
SciPy 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
SciPy member

OK, thanks Josef. Pushed.

@rgommers rgommers closed this Dec 20, 2011
@calbaker

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

@rgommers
SciPy member

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