Thank you @melissawm !

Could someone remind me what is the motivation of replacing np.dot with the BLAS call in this particular case? I understand the use case for combining multiple operations in a single BLAS call as in #11507 but that's not the case here. Is it that it would release the GIL or that there are less pre-processing?

@melissawm Could you please

• benchmark ElasticNet before and after this change
• confirm that the way this function is used Q and w is, I assume, always C contiguous, meaning that it was allocated somewhere before in the code, and is not for instance is a user input or obtained by array slicing.

Actually, should we check that the array is C contiguous before applying BLAS functions? Since code can change in the future.

Q and w are both contiguous (with the C-layout for Q as declared in the prototype of the enet_coordinate_descent_gram Cython function:

floating[::1] w

and

np.ndarray[floating, ndim=2, mode='c'] Q

that being said, I am not sure we would get a performance improvement in this specific situation. Maybe @jeremiedbb knows? In any case, a small benchmark would give us a definite answer.

BTW, the GIL is already released at line 540. Neither the np.dot version nor the _gemv version need the GIL in this case.

The motivation of the mentioned issue is to use higher level blas functions than the ones currently used when possible, i.e. use gemv instead of dots in a loop for instance, not replace numpy dot par directly calling the corresponding blas function. In would not expect much speed-up in that case, although a benchmark is welcomed because I could be wrong.

Yeah I would agree with that. It's possible the list I made (a long time ago) included this line erroneously

Got it - this is all helpful information so I'll review all this with benchmarks. I'll mark this one as a draft for completeness and post the benchmarking results here, but you are right that there may not be an improvement. In any case it is possible the I can go thought the list and find other stuff and now I know what to look for. Thanks!

+1 to implement other cases from #13210 (comment) except for this one.

#DataUmbrella LATAM sprint

Sent @melissawm reminder on PR.

This PR is stalled, and is open to a contributor.

I have relabeled this PR as per this comment.
Feel free to modify if I am wrong.

I see 2 possible ways forward:

1. Put the initialization of H just after with nogil and merge - we should not ask to make extensive benchmarks here as we do not except a (well measurable) improvement.
2. Close this PR (which I prefer over open ends).

@melissawm IMO, it's up to you if you are still interested - and apologies for the long decision processes here:blush:

If there is no measurable improvement, then I prefer to close. np.dot is easier for future contributors & maintainers to reason about.

np.dot is easier for future contributors & maintainers to reason about.

I agree, and as a general rule of thumb, directly calling BLAS should be done only where we don't want any python interaction at all. Elsewhere, numpy should be preferred, unless a convincing benchmarks shows otherwise.

I'm also in favor of closing since it's not benchmarked (and very unlikely to give a speed-up).

So let's close.