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

allow any AbstractVector in roc #25

Closed
bkamins opened this issue Feb 21, 2022 · 5 comments
Closed

allow any AbstractVector in roc #25

bkamins opened this issue Feb 21, 2022 · 5 comments

Comments

@bkamins
Copy link
Contributor

bkamins commented Feb 21, 2022

Currently you have:

roc(tar::Vector{T}, non::Vector{T}; laplace, collapse) where T<:Real

which is quite restrictive.

Would you consider loosening the signature to:

roc(tar::AbstractVector, non::AbsrtractVector; laplace, collapse)

and internally check if tar and non contain only Real values?

This would allow:

  • any abstract vector, mainly views
  • having tar and non element type to be e.g. Union{Missing, Float64} which is often hit in practice when processing data (i.e. column allows missings, but actually it does not have it as user has manually dropped them, but without changing eltype of column)

Thank you!

@davidavdav
Copy link
Owner

Hello,

Going from Vector to AbstractVector is definitely the way forward. As for the type of the element, sure we can relax that, but this makes the code somewhat slower, right? Maybe we can have an additional relaxed signature that does the checking and then calls the strict signature?

@bkamins
Copy link
Contributor Author

bkamins commented Feb 22, 2022

Maybe we can have an additional relaxed signature that does the checking and then calls the strict signature?

Yes

@davidavdav
Copy link
Owner

I pushed a new version. It appears that Travis is no longer working, so I don't know how to deal with CI right now.

Maybe you can test it.

@bkamins
Copy link
Contributor Author

bkamins commented Feb 22, 2022

you need to switch to GitHub Actions. I do not know exactly how to do it, it should be relatively simple.

The commit looks good. Thank you!

@bkamins
Copy link
Contributor Author

bkamins commented Feb 23, 2022

Thank you!

@bkamins bkamins closed this as completed Feb 23, 2022
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

No branches or pull requests

2 participants