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

Add 'auto' option for populate_phys_input chtrig parameter #257

Closed
3 tasks
tsalo opened this issue Jun 18, 2020 · 7 comments · Fixed by #306
Closed
3 tasks

Add 'auto' option for populate_phys_input chtrig parameter #257

tsalo opened this issue Jun 18, 2020 · 7 comments · Fixed by #306
Labels
Enhancement New feature or request released This issue/pull request has been released.

Comments

@tsalo
Copy link
Member

tsalo commented Jun 18, 2020

Users may not know which channel is the trigger channel, so it would help them to be able to auto-detect the trigger channel and, in the absence of useable information, raise an error with the names of the available channels.

Context / Motivation

For any user who doesn't know/remember what their trigger channel is, they have to load the data to a BlueprintInput with some random number set as the trigger channel, check the channel names, and then re-load the data with the correct trigger channel index. This should streamline the process in those cases. Plus, it would simplify the standard loading procedure in that there would only be one required argument to the function.

Possible Implementation

  • Create an alias list with "trig", "trigger", "TRIGGER", [...]
  • raise a warning if the trigger channel's name does not contain any element in the alias list
  • if the user didn't give a trigger channel, we look into the names and check if there is a match to any element of the alias list. If there is not, we raise an error.
@eurunuela
Copy link
Collaborator

Is this related to #204 ?

@smoia
Copy link
Member

smoia commented Jun 18, 2020

This seems to be half way between #204 and the reason we have -info as an argument (to check files before running batches of phys2bids). What does it add on top of the behaviour of phys2bids -info option?

@tsalo
Copy link
Member Author

tsalo commented Jun 18, 2020

I wasn't aware of -info when I opened the issue (my bad!), but there are still going to be users who don't use the workflow. Regarding #204, this is a much simpler solution to a much smaller problem. Instead of looking at the signal, this just looks at the channel names.

@smoia
Copy link
Member

smoia commented Jun 18, 2020

What do you think if instead:

a. we create an alias list with "trig", "trigger", "TRIGGER", [...]
b. we raise a warning if the trigger channel's name does not contain any element in the alias list
c. if the user didn't give a trigger channel, we look into the names and check if there is a match to any element of the alias list. If there is not, we raise an error.

Reasoning: During data acquisition it's not granted that you are calling your channels "trig". If you don't have it in your channel names, rasing an error will make phy2bids not able to do its job. However, we can help a bit the user.

@tsalo
Copy link
Member Author

tsalo commented Jun 18, 2020

Absolutely. This would be perfect! So there's still an 'auto' option, but it's far more robust than what I originally planned, right? Plus a new check even when the trigger channel is provided.

@smoia smoia added the Enhancement New feature or request label Jul 30, 2020
@smoia smoia added this to the phys2bids second semester 2020 milestone Jul 30, 2020
@vinferrer
Copy link
Collaborator

I think this is a good thing to implement, however we are gonna have to wait for #273. Since it will affect the chtrig.

@smoia
Copy link
Member

smoia commented Feb 17, 2021

🚀 Issue was released in 2.4.0 🚀

@smoia smoia added the released This issue/pull request has been released. label Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants