make nystrom work with AbstractVector

[st--]

Resolves #426

[codecov]

Codecov Report

Merging #427 (6d99f21) into master (d1c68a9) will increase coverage by 0.35%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #427      +/-   ##
==========================================
+ Coverage   92.74%   93.09%   +0.35%     
==========================================
  Files          52       52              
  Lines        1199     1202       +3     
==========================================
+ Hits         1112     1119       +7     
+ Misses         87       83       -4     

Impacted Files	Coverage Δ
src/approximations/nystrom.jl	92.50% <100.00%> (+0.60%)	⬆️
src/utils.jl	90.14% <0.00%> (+5.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1c68a9...6d99f21. Read the comment docs.

[devmotion]

Is only nystrom part of the API? Then it would be sufficient to support and wrap matrices there and only use ColVecs etc internally.

[st--]

Is only nystrom part of the API? Then it would be sufficient to support and wrap matrices there and only use ColVecs etc internally.

I would consider only nystrom to be part of the public API, it's the only exported method, but as there's nothing that would have stopped downstream users from calling KernelFunctions.nystrom_sample, and as it's easy to have the same additional wrapper there for backwards compatibility, I would suggest to leave it as is.

[devmotion]

OK, seems only nystrom is exported:

KernelFunctions.jl/src/KernelFunctions.jl

Line 33 in d1c68a9

export NystromFact, nystrom

Then I'd suggest deprecating at least the other entry points for matrices (of course, without exporting them; you can use @deprecate f(...) f_new(...) false) to remind us that we should remove them eventually. Removing them will make the code simpler and since they are not public there is no need to support all possible inputs.

[st--]

@devmotion I've made the change you suggested; are you happy to approve now (or if not, could you please clarify what changes you want to see before approving it, or whether you're just commenting but unwilling to finish the review)? thanks:)

[devmotion]

I don't think it's good to put pressure on anyone to review and/or approve a PR as fast as possible, in particular if it is not urgent at all 🧘 I try to be reasonably responsive (and if I forget something which can happen, it's fine to ping me) but ultimately I decide when and if I comment or review. And not even this is true currently - I'm only at my phone throughout the day and that only if my daughter is asleep 😄 So quite naturally it takes a while until I (can) review and/or approve a PR.

[st--]

I don't think it's good to put pressure on anyone to review and/or approve a PR as fast as possible, in particular if it is not urgent at all lotus_position

The reason I came across this issue and fixed it was because I actually needed it for myself, it was urgent for me...

ultimately I decide when and if I comment or review.

Yes, of course. That's why I asked whether you actually wanted to review it.

If no-one has reviewed, I can approach people and see who might have time to go through & review it.
If someone jumps on it and reviews it and says what they think is missing to get it ready to merge, that's great.

What I find personally very unhelpful and frustrating is to get some comments on a PR, seeming like someone already went through to review it (potentially putting off other potential reviewers), but with no clear indication whether it was a 'full' review and those are all the comments that need addressing, or whether it was just a few random comments on some aspects of it but not an actual review. It also leaves it ambiguous whether you would agree or oppose it if someone else goes ahead and approves it.

What I would like to get is a clear indication of what action I can do next: fix something? approach others to review? just wait because it's really important to you to review it, but you don't have time right away? - any of those is better than just wondering what's holding up my proposed changes.

[theogf]

Just a few comments to fix (for NystromFact it's up to you) and LGTM

[devmotion]

LGTM.