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

Remove input check for vector of vectors #513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grero
Copy link

@grero grero commented May 31, 2023

Summary

This attempts to address #512, which prevents kernels operating on vectors of unequal length from working properly.

Proposed changes

  • ...
    Remove the check on input dimensions when the inputs are vectors of vectors, since there is nothing wrong in principal with having kernels operate on these kinds of inputs.

What alternatives have you considered?

The most obvious alternative would be to implement my own type, wrapping AbstractVector{<:AbstractVector{<:Real}}, but since this change would potentially benefit others with similar needs, a fix in KernelFunctions itself seemed more appropriate.
Breaking changes

The main change is that inputs checks that are currently failing would pass. This is potentially breaking, as it is possible that downstream code could rely on these checks.

@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch coverage has no change and project coverage change: -14.31 ⚠️

Comparison is base (ef6d459) 94.16% compared to head (6b153d9) 79.85%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #513       +/-   ##
===========================================
- Coverage   94.16%   79.85%   -14.31%     
===========================================
  Files          52       52               
  Lines        1387     1385        -2     
===========================================
- Hits         1306     1106      -200     
- Misses         81      279      +198     
Impacted Files Coverage Δ
src/utils.jl 69.87% <ø> (-21.59%) ⬇️

... and 18 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@willtebbutt
Copy link
Member

willtebbutt commented May 31, 2023

Thanks for opening this PR.

I understand why you've taken this approach, but I don't believe it's the correct approach, as whether or not the input-dimension comparison should be performed is a function of the kernel, not the input type. For example, it (should be) perfectly fine to feed a Vector{Vector{Float64}} into an SEKernel, and in that case we would generally like to check that the input dimensions match.

To my mind, a better implementation would be to switch the check on / off depending upon which kernel is employed.

@theogf @devmotion @st-- do any of you have thoughts on this problem? I'm starting to wonder whether having this check is more hassle than it's worth, because the fix looks like it's going to be a bit tricky to implement.

@devmotion
Copy link
Member

To my mind, a better implementation would be to switch the check on / off depending upon which kernel is employed.

Yes, I agree, that seems to be the best and most general approach. Maybe also dim is too specific and instead we should add a function such as are_valid_inputs(kernel, x, y)::Bool as the basis of valid_inputs?

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

Successfully merging this pull request may close these issues.

3 participants