BUG: make curve_fit() work with array_like input. Closes gh-3037. #3166

Merged
merged 1 commit into from Dec 26, 2013

Conversation

Projects
None yet
5 participants
@rgommers
Member

rgommers commented Dec 20, 2013

No description provided.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 20, 2013

Coverage Status

Coverage remained the same when pulling 856e396 on rgommers:curvefit-list-input into 34ae412 on scipy:master.

Coverage Status

Coverage remained the same when pulling 856e396 on rgommers:curvefit-list-input into 34ae412 on scipy:master.

@dlax

This comment has been minimized.

Show comment
Hide comment
@dlax

dlax Dec 21, 2013

Member

Looks good to me. Does it work for other people involved in the issue discussion? @josef-pkt

Member

dlax commented Dec 21, 2013

Looks good to me. Does it work for other people involved in the issue discussion? @josef-pkt

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Dec 21, 2013

Member

It still looks dangerous to me, I would maybe take out the tuple and only convert list (if I would convert anything at all).

I don't have any code using curve_fit (except script to try it out while looking at issues for it).
The only one I remember recently from stackoverflow is xdata = (x1, x2) and tuple unpacking inside the user function, which wouldn't break with this change, just make an unnecessary array conversion.

I also don't know if anyone using [x1, x2] actually would like column_stack([x1, x2]) instead of row stacking.

(I would just change the starting values to 0.999 and let the user deal with the exceptions in his/her code.
and maybe go for deprecation default starting values as has been suggested.)

Changing this is just guessing on what users might be doing. I'm just pointing out some cases, and have no idea what different users are actually doing. which essentially means I'm -0 on this.

Member

josef-pkt commented Dec 21, 2013

It still looks dangerous to me, I would maybe take out the tuple and only convert list (if I would convert anything at all).

I don't have any code using curve_fit (except script to try it out while looking at issues for it).
The only one I remember recently from stackoverflow is xdata = (x1, x2) and tuple unpacking inside the user function, which wouldn't break with this change, just make an unnecessary array conversion.

I also don't know if anyone using [x1, x2] actually would like column_stack([x1, x2]) instead of row stacking.

(I would just change the starting values to 0.999 and let the user deal with the exceptions in his/her code.
and maybe go for deprecation default starting values as has been suggested.)

Changing this is just guessing on what users might be doing. I'm just pointing out some cases, and have no idea what different users are actually doing. which essentially means I'm -0 on this.

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Dec 21, 2013

Member

@josef-pkt: a better way than 0.999 is to replace params with params.tolist() in _general_function and _weighed_general_function. Just changing the value doesn't help because np.float64(0.999) * [1,2,3] == [].

However, this doesn't solve the issue that in the most common use case where x is meant to be an array, passing in list doesn't work.

Member

pv commented Dec 21, 2013

@josef-pkt: a better way than 0.999 is to replace params with params.tolist() in _general_function and _weighed_general_function. Just changing the value doesn't help because np.float64(0.999) * [1,2,3] == [].

However, this doesn't solve the issue that in the most common use case where x is meant to be an array, passing in list doesn't work.

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Dec 21, 2013

Member

Just changing the value doesn't help because np.float64(0.999) * [1,2,3] == [].

I think it would avoid the original case where curve_fit returned wrong results instead of raising an exception.

I agree that in most cases users that use a list actually want an array. I'm less convinced for the tuple case.
Also if we convert only list, then users can still easily pass in other "things", which was the main part of my original objection.

executive decision required: guessing what users want (which will be correct in most cases) or being more liberal with exceptions

Member

josef-pkt commented Dec 21, 2013

Just changing the value doesn't help because np.float64(0.999) * [1,2,3] == [].

I think it would avoid the original case where curve_fit returned wrong results instead of raising an exception.

I agree that in most cases users that use a list actually want an array. I'm less convinced for the tuple case.
Also if we convert only list, then users can still easily pass in other "things", which was the main part of my original objection.

executive decision required: guessing what users want (which will be correct in most cases) or being more liberal with exceptions

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 21, 2013

Member

Raising exceptions instead of returning wrong/unexpected results is also not acceptable I think. This should just work, like for any other function with array_like input.

Anything else, like passing in random objects that the user-provided function then knows how to handle, is nice but of secondary importance imho.

Member

rgommers commented Dec 21, 2013

Raising exceptions instead of returning wrong/unexpected results is also not acceptable I think. This should just work, like for any other function with array_like input.

Anything else, like passing in random objects that the user-provided function then knows how to handle, is nice but of secondary importance imho.

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Dec 21, 2013

Member

Still we are just guessing on the usage.

Reminds me that curve_fit originally only allowed functions and broke with a method attached to a class (in inspect).
Which also reminds me that there is a way to get arbitrary content into the user function, either by using a class or reusing things from the outer scope like in nested functions. Which means there is another work around if users don't want the array conversion.

Member

josef-pkt commented Dec 21, 2013

Still we are just guessing on the usage.

Reminds me that curve_fit originally only allowed functions and broke with a method attached to a class (in inspect).
Which also reminds me that there is a way to get arbitrary content into the user function, either by using a class or reusing things from the outer scope like in nested functions. Which means there is another work around if users don't want the array conversion.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 21, 2013

Member

Besides common sense, there's a bunch of confused users on Stackoverflow. I wouldn't call that just guessing.

For anything that is not a list/tuple, there is still no array conversion after merging this PR. So no workaround needed, even though those are possible.

Member

rgommers commented Dec 21, 2013

Besides common sense, there's a bunch of confused users on Stackoverflow. I wouldn't call that just guessing.

For anything that is not a list/tuple, there is still no array conversion after merging this PR. So no workaround needed, even though those are possible.

@pv

This comment has been minimized.

Show comment
Hide comment
@pv

pv Dec 21, 2013

Member

Well, the documentation states "xdata : An N-length sequence or an (k,N)-shaped array". I believe it would be fine with an unconditional xdata = np.asanyarray(xdata). (Although, people, might pass in pandas dataframes, so just dealing with lists and tuples is probably OK, although quite ugly.)

Member

pv commented Dec 21, 2013

Well, the documentation states "xdata : An N-length sequence or an (k,N)-shaped array". I believe it would be fine with an unconditional xdata = np.asanyarray(xdata). (Although, people, might pass in pandas dataframes, so just dealing with lists and tuples is probably OK, although quite ugly.)

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 21, 2013

Member

That would also be OK with me.

Member

rgommers commented Dec 21, 2013

That would also be OK with me.

@josef-pkt

This comment has been minimized.

Show comment
Hide comment
@josef-pkt

josef-pkt Dec 21, 2013

Member

Ok I'm +0, since there are easy ways to work around.

Member

josef-pkt commented Dec 21, 2013

Ok I'm +0, since there are easy ways to work around.

@dlax

This comment has been minimized.

Show comment
Hide comment
@dlax

dlax Dec 22, 2013

Member

So the consensus is on "unconditional asanyarray"?

Member

dlax commented Dec 22, 2013

So the consensus is on "unconditional asanyarray"?

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 22, 2013

Member

I'm OK either way but still think the current PR makes the most sense, to not break any current off-label usage (classes, pandas dataframes, etc.). That can be documented and tested explicitly if it helps this PR go in.

Member

rgommers commented Dec 22, 2013

I'm OK either way but still think the current PR makes the most sense, to not break any current off-label usage (classes, pandas dataframes, etc.). That can be documented and tested explicitly if it helps this PR go in.

dlax added a commit that referenced this pull request Dec 26, 2013

Merge pull request #3166 from rgommers/curvefit-list-input
BUG: make curve_fit() work with array_like input.  Closes gh-3037.

@dlax dlax merged commit 3982a90 into scipy:master Dec 26, 2013

1 check passed

default The Travis CI build passed
Details

@rgommers rgommers deleted the rgommers:curvefit-list-input branch Dec 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment