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

make cfg.channels forbidden #1729

Closed
robertoostenveld opened this issue Apr 7, 2021 · 7 comments
Closed

make cfg.channels forbidden #1729

robertoostenveld opened this issue Apr 7, 2021 · 7 comments

Comments

@robertoostenveld
Copy link
Contributor

Is your feature request related to a problem? Please describe.

I noticed a user to specify cfg.channels rather than cfg.channel, where the channel selection was subsequently ignored. This lead to unexpected results further down in the analysis.

Describe the solution you'd like

Using ft_checkconfig we could make cfg.channels forbidden in all high-level functions.

Describe alternatives you've considered

We could also make use the ft_checkconfig renamed option and rename channels into channel. Since channels was never used, I am not in favor of that.

@schoffelen
Copy link
Contributor

I agree. Better make the user aware of the fact that they should pay attention to the proper spelling of options, rather than automatic fixes for often unintended typos

@robertoostenveld
Copy link
Contributor Author

The same thing just happened for cfg.method rather than cfg.interpmethod in ft_sourceinterpolate.

@schoffelen
Copy link
Contributor

regarding method: there's also a cfg.planarmethod :). Would it not make sense to revert those instances back to plain cfg.method? in case the function does not have different methods as option, it would streamline the option naming scheme in general.

@schoffelen
Copy link
Contributor

BTW, regarding channels vs. channel: why is it called cfg.trials, and not cfg.trial?

@robertoostenveld
Copy link
Contributor Author

there is also invmethod, pairmethod, mvarmethod, submethod, refmethod, cohmethod, powmethod, supmethod, fwhmmethod, projmethod, spmmethod, and others.

For some it is a sub-specification, for others it is the consequence of code refactoring in the past, e.g. ft_megplanar was once part of the same high-level function as ft_meginterpolate.

For functions that have a single unambiguous method, like ft_sourceinterpolate, I do prefer cfg.method over the peculiar methods that are now used.

@schoffelen
Copy link
Contributor

pairmethod?

@robertoostenveld
Copy link
Contributor Author

regarding interpmethod: I see in ft_sourceplot that when cfg.method=surface you can specify cfg.projmethod, which later in the code is translated into tmpcfg.interpmethod.

I think that is too big to be solved together with this issue. We can keep it in mind for later.

robertoostenveld added a commit that referenced this issue Apr 7, 2021
FIX #1729 - make cfg.channels explicitly forbidden
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants