Skip to content

MAINT: Implement lstsq as a gufunc#9980

Merged
mhvk merged 4 commits intonumpy:masterfrom
eric-wieser:linalg-lstsq-ufunc
Apr 11, 2018
Merged

MAINT: Implement lstsq as a gufunc#9980
mhvk merged 4 commits intonumpy:masterfrom
eric-wieser:linalg-lstsq-ufunc

Conversation

@eric-wieser
Copy link
Copy Markdown
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
Copy Markdown
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 Outdated
Copy link
Copy Markdown
Contributor

@mhvk mhvk Nov 10, 2017

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
Copy Markdown
Member Author

@eric-wieser eric-wieser Nov 10, 2017

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
Copy Markdown
Contributor

@mhvk mhvk Nov 10, 2017

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
Copy Markdown
Member Author

eric-wieser commented Nov 10, 2017

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
Copy Markdown
Member Author

eric-wieser commented Nov 10, 2017

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
Copy Markdown
Member Author

eric-wieser commented Nov 10, 2017

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

eric-wieser commented Apr 11, 2018

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

@mhvk
Copy link
Copy Markdown
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
Copy Markdown
Member Author

eric-wieser commented Apr 11, 2018

Would you consider #9516 to be that issue?

@mhvk mhvk mentioned this pull request Apr 11, 2018
Closed
@mhvk
Copy link
Copy Markdown
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