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

BUG: linalg.norm(): Don't convert object arrays to float #7587

Merged
merged 2 commits into from
May 7, 2016

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Apr 29, 2016

This fixes #7575 (Regression in linalg.norm() using dtype=object).

Please tell me if I shall rebase this onto another branch.

This fixes numpy#7575 (Regression in linalg.norm() using dtype=object).
@@ -90,6 +90,12 @@ def test_svd_no_uv(self):
assert_equal(np.linalg.matrix_rank(a), 1)
assert_array_less(1, np.linalg.norm(a, ord=2))

def test_norm_object_array(self):
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to test more of the norms, or at least all that work with this fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added more tests in bfa3ed9d29314d9d4a06eb5172ae855d19ae7c8d.

Everything that was broken before the regression is still broken.

@charris
Copy link
Member

charris commented Apr 30, 2016

I've added a comment to the issue #7575.

@charris
Copy link
Member

charris commented May 3, 2016

I might put this in 1.11.1, but not 1.12, to give you more time to make your own version of norm and get that out there. Deal?

@mgeier
Copy link
Contributor Author

mgeier commented May 4, 2016

If it's definitely going to be dropped for 1.12, I don't care about what's happening in 1.11.1.
I could very quickly switch to the work-around given by @pv in #7575.

But I don't think this would be a good decision for the linalg submodule to demote object arrays from being first-class citizens.

Let me offer a counter-deal:
You add my fix to 1.11.1, in the meantime I'll try to come up with a PR that adds support for object arrays in numpy.linalg.norm() for all possible values of the ord keyword.
If you are satisfied with my PR, it goes into 1.12. If not (or if I can't come up with one until the deadline for 1.12), you can drop the whole thing (and we are back to your deal from above).

What about that?

@charris
Copy link
Member

charris commented May 7, 2016

Let's put this in with the understanding that it is something of an implementation detail. If you want a guarantee that your work will not be affected by future changes you should pursue your own implementation of a norm function that works as you need it to for your application.

@charris charris merged commit c515fcd into numpy:master May 7, 2016
@charris
Copy link
Member

charris commented May 7, 2016

Thanks @mgeier .

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

Successfully merging this pull request may close these issues.

Regression in linalg.norm() using dtype=object
3 participants