Skip to content

MAINT: Implement lstsq as a gufunc #9980

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 11, 2018
Merged

MAINT: Implement lstsq as a gufunc #9980

merged 4 commits into from
Apr 11, 2018

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Nov 7, 2017 •
edited
Loading

A summary of what this does

  • Includes the missing s and c varaiants of gelsd (not used by lstsq, but there if people need them) (the very large diffs are simply what happens when regenerating lapack_lite)
  • Writes the C wrappers that call the lapack functions and allocate/deallocate their workspaces
  • Changes the implementation of lstsq to fall back on one of two gufuncs (that share a loop)

What this does not try to do yet, since there are decisions to be made for these:

Like #9976 (merged), this needs lapack 3.2.2.

This was tested for exact equality by placing asserts within lstsq comparing old and new values, and rerunning the test suite.

@eric-wieser eric-wieser force-pushed the linalg-lstsq-ufunc branch 6 times, most recently from 341536c to 110de75 Compare November 10, 2017 04:28
@eric-wieser eric-wieser requested a review from mhvk November 10, 2017 04:34
@eric-wieser eric-wieser changed the title WIP/ENH: Implement lstsq as a gufunc MAINT: Implement lstsq as a gufunc Nov 10, 2017
mhvk
mhvk approved these changes Nov 10, 2017
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

I very much like this! And it all looks great. Ideally, we allow lstsq now to take single precision inputs, but that is probably better done in a follow-up PR as well (not sure how best to handle it; new argument to tell whether to allow single precision?)

Relatedly, since we expose the single-precision routines, we would ideally also test those, but I think that is also fine to leave to a next PR.

numpy/linalg/linalg.py
b_out = bstar.T
gufunc = _umath_linalg.lstsq_n

signature = 'DDd->Did' if isComplexType(t) else 'ddd->did'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary given that the inputs are coerced already?

Copy link
Member Author

Choose a reason for hiding this comment

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

The inputs don't look coerced already to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, above one just gets the types for later conversion. Should have noticed that.

@eric-wieser
Copy link
Member Author

Ideally, we allow lstsq now to take single precision inputs

I think we're stuck with not doing this, in the same way that svd is

@eric-wieser
Copy link
Member Author

An earlier version exposed twice as many ufuncs, lstsq_m, and lstsq1_m where the latter was a specialization for just vectors. There's precedent for this in solve, but it seemed clearer just to insert extra dimensions within the python code.

@eric-wieser
Copy link
Member Author

Once this goes in, I have another commit that promotes resids to a proper ufunc return, which saves some memory, but at the expense of clarity.

@mhvk
Copy link
Contributor

mhvk commented Nov 10, 2017

I think in principle we could add a dtype argument where the default is float or complex but can be explicitly set to whatever one wants. Or simply a allow_single_precision or something like it. Possibly, a linalg wide context manager could do the trick too. Anyway, for later...

@eric-wieser
Copy link
Member Author

eric-wieser commented Nov 10, 2017 •
edited
Loading

Possibly, a linalg wide context manager could do the trick too

Of course, then we run into #9444.

I'd be in favor of allowing single-precision via dtype kwarg, with a default of dtype='promote' - passing dtype = None would override this to do the normal ufunc thing. Lets leave this till another time though.

@eric-wieser eric-wieser force-pushed the linalg-lstsq-ufunc branch 4 times, most recently from 729e6eb to 89bca66 Compare April 11, 2018 06:29
@eric-wieser
Copy link
Member Author

Rebased, and slightly squashed. Good to go, @mhvk?

@mhvk
Copy link
Contributor

mhvk commented Apr 11, 2018

Yes, this looks good, modulo that there should at least be new issues reminding us to introduce a dtype option to get single-precision and tests for the single-precision routines (which can be addressed in the same PR of course).

@eric-wieser
Copy link
Member Author

Would you consider #9516 to be that issue?

@mhvk mhvk mentioned this pull request Apr 11, 2018
Closed
@mhvk
Copy link
Contributor

mhvk commented Apr 11, 2018

I think a new one is better (more focused); see #10888 . Will now merge this one.

@mhvk mhvk added this to the 1.15.0 release milestone Apr 11, 2018
@mhvk mhvk merged commit 265983b into numpy:master Apr 11, 2018
@mhvk mhvk mentioned this pull request Apr 12, 2018
Open
@eric-wieser eric-wieser mentioned this pull request Oct 20, 2018
Closed
@eric-wieser eric-wieser mentioned this pull request May 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk approved these changes

Assignees
No one assigned
Labels
01 - Enhancement 03 - Maintenance component: numpy.linalg
Projects
None yet
Milestone
1.15.0 release
Development

Successfully merging this pull request may close these issues.
2 participants
@eric-wieser @mhvk