unfoldtoolbox / Unfold.jl

Neuroimaging (EEG, fMRI, pupil ...) regression analysis in Julia
MIT License
48 stars 10 forks source link

Inconsistency in predict( ) input; better docstring #216

Closed ReneSkukies closed 1 month ago

ReneSkukies commented 2 months ago

Currently, there is an inconsistency in the input of the predict() function when using the keep_basis and exclude_basis keywords:

# Example 1; this one works
predict(
    m,
    Unfold.formulas(m),
    Unfold.events(m);
    overlap = true,
    keep_basis = ["basis_one"]
)

# Example 2; this one also works
predict(
    m,
    Unfold.formulas(m),
    Unfold.events(m);
    overlap = true,
    exclude_basis = "basis_two"
)

# Example 3; this one does not work
predict(
    m,
    Unfold.formulas(m),
    Unfold.events(m);
    overlap = true,
    keep_basis = "basis_one"
)

I think this should either be consistent or made apparent to the user.

The error results from the matrix_by_basisname(::AbstractMatrix, ::Any, !Matched::Vector) only accepting a vector as input.

I think the easiest solution would be to force the user to use a vector from the start and provide useful error messages. The other option would be to convert to a vector. Either way this should also be made apparent in the docstring of predict()

ReneSkukies commented 2 months ago

@behinger if you let me know what you prefer, I can make a pull request ๐Ÿ‘๐Ÿผ

behinger commented 1 month ago

ah thanks for catching. Indeed, I think I like the convenience function to wrap a single string input into a predictor like in exclude_basis

if you want to implement: ๐Ÿ™‡

ReneSkukies commented 1 month ago

Working on it on the update_predict branch ๐Ÿ‘๐Ÿผ