Auto-detect the trigger channel based on retrieved channel names #306

vinferrer · 2020-09-18T12:18:41Z

Closes #257
auto-detect the trigger channel comparing ch_name list to a of possible trigger names

Proposed Changes

Default chtrig is now 0 which will trigger the auto option
Create an alias list with possible trigger names
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.

vinferrer · 2020-09-18T12:38:42Z

Hello, @eurunuela, @tsalo, @smoia. I have implemented the auto option for the chtrig inside BlueprintInput class, because for me this makes more sense since it is the moment we start using the chtrig parameter. I left this as draft because i wan't you to corroborate that it makes sense to implement here the functionality the main reason is that independently of which interface we are running they all endup dropping the data in this class so no need for duplicities.
Also feel free to suggest new names for the chtrig alias names.

phys2bids/physio_obj.py

tsalo · 2020-09-18T12:51:40Z

I'll leave the discussion of the proper index for the trigger channel (i.e., 0 or 1) and where that switch happens to others, but I had some minor code edits/comments. Thanks for this!

Co-authored-by: Taylor Salo <[email protected]>

smoia

I can't wait to see this implemented and working!

However, I have few comments and changes to ask (see comments).

There's another thing: I like the fact that this functionality is implemented in the object methods, but I don't know if it's actually good to have it there, and in any case I wouldn't put it in the initialisation of the object.

The reason for which I'm not sure it's good to have it in the object is that I think we're using the trigger channel to extract the time array when we deal with acq files (in the respective interface script), and that's a step that takes place before the initialisation of the object.
Maybe a better place would be the (refactored) interfaces script/folder, with this step taking place before the input processing. It's not vital though, we can just specify the change in the initialisation of acq files.

If we want to leave it as an object method (and I see why), I would make it a separate method and call it in case chtrig is zero, even after the initialisation of the object. It just means updating its properties.

phys2bids/cli/run.py

phys2bids/interfaces/txt.py

phys2bids/phys2bids.py

phys2bids/physio_obj.py

eurunuela · 2020-09-18T13:38:46Z

Hi @vinferrer , thanks a lot for this! I agree with the comments from @tsalo and @smoia . My other concern would be the linting not passing the test. I think once the comments are added we can merge it. Thanks again!

Co-authored-by: Stefano Moia <[email protected]>

codecov · 2020-09-18T14:14:27Z

Codecov Report

Merging #306 (2046244) into master (bdf4480) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
+ Coverage   94.63%   94.77%   +0.14%     
==========================================
  Files           8        8              
  Lines         839      862      +23     
==========================================
+ Hits          794      817      +23     
  Misses         45       45

Impacted Files	Coverage Δ
phys2bids/cli/run.py	`96.55% <ø> (ø)`
phys2bids/phys2bids.py	`88.13% <100.00%> (+0.06%)`	⬆️
phys2bids/physio_obj.py	`93.75% <100.00%> (+0.73%)`	⬆️

vinferrer · 2020-09-21T08:32:35Z

Okay i have

I can't wait to see this implemented and working!

However, I have few comments and changes to ask (see comments).

There's another thing: I like the fact that this functionality is implemented in the object methods, but I don't know if it's actually good to have it there, and in any case I wouldn't put it in the initialisation of the object.

The reason for which I'm not sure it's good to have it in the object is that I think we're using the trigger channel to extract the time array when we deal with acq files (in the respective interface script), and that's a step that takes place before the initialisation of the object.
Maybe a better place would be the (refactored) interfaces script/folder, with this step taking place before the input processing. It's not vital though, we can just specify the change in the initialisation of acq files.

If we want to leave it as an object method (and I see why), I would make it a separate method and call it in case chtrig is zero, even after the initialisation of the object. It just means updating its properties.

I see what you mean about acq files. Is it critical that you use the chtrig channel to obtain the frequency and the time index?

vinferrer · 2020-09-21T09:07:24Z

So guys I have implemented the partial match thingy as you can see, if you don't want me to have this in physio_obj.py we could put it as a utils function that is used in each of the interfaces.py script but i think that would make more duplicities. Is there a way we can avoid using chtrig in acq.py?

eurunuela · 2020-09-21T09:10:33Z

So guys I have implemented the partial match thingy as you can see, if you don't want me to have this in physio_obj.py we could put it as a utils function that is used in each of the interfaces.py script but i think that would make more duplicities. Is there a way we can avoid using chtrig in acq.py?

I agree that not using chtrig in acq.py would make the code cleaer. Any idea @smoia ? I believe that piece of code was yours.

tsalo · 2021-02-04T15:27:22Z

You're right. It would require a couple of changes to the CLI, although we have a good template for that kind of argument in tedana now, with --tedpca. See here.

If it seems like too much work, or otherwise just not a good idea, what about -1? Most Python users would think 0 would be a standard value, while -1 is a common "special value".

vinferrer · 2021-02-04T21:48:05Z

Okay, actually I do have a very strong opinion about using auto or -1 as the default option, think that the check is done after reading the file, all our io.py are designed to use chtrig as a number and there is actually an error designed to pop up when chtrig is negative, so I would stay with the 0 as default option

tsalo

LGTM!

phys2bids/physio_obj.py

smoia · 2021-02-16T11:18:37Z

I checked again the regex comparison, and came up with the alternative solution (see this comment) that doesn't have the issue of matching wrong channel names due to empty strings in the channel name split.
Granted, it's less elegant because it uses more lines of code, but it's a bit more specific.
I opened a PR to your branch, here if @tsalo or @eurunuela want to have a look at it.

Co-authored-by: Stefano Moia <[email protected]>

…uto_chtrig

…into auto_chtrig

Propose less elegant but more specific comparison

vinferrer · 2021-02-17T08:04:57Z

Well I think we are ready to merge this @smoia

smoia

LGTM, but I'm biased!
@tsalo @eurunuela do you want to give a second look? If so, the last one can merge this in!

tsalo

LGTM

smoia · 2021-02-17T17:41:50Z

🚀 PR was released in 2.4.0 🚀

vinferrer added 3 commits September 17, 2020 16:13

default chtrig is 0

62ec35e

default chtrig is also 0

04bd62d

add automatic recognition of a trigger channel by name

c30ac75

vinferrer requested review from tsalo, eurunuela and smoia September 18, 2020 12:18

vinferrer assigned eurunuela Sep 18, 2020

vinferrer marked this pull request as draft September 18, 2020 12:18

vinferrer added 2 commits September 18, 2020 14:24

add warking if trigger channel name doesn't match any alias

bf37e1f

change 'Wrong trigger' chtrig to -1

23bf7f4

tsalo reviewed Sep 18, 2020

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

tsalo reviewed Sep 18, 2020

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

tsalo reviewed Sep 18, 2020

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

tsalo reviewed Sep 18, 2020

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

Update phys2bids/physio_obj.py

e43b51a

Co-authored-by: Taylor Salo <[email protected]>

smoia requested changes Sep 18, 2020

View reviewed changes

edit warning messages

a4dff32

smoia reviewed Sep 18, 2020

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

merge with remote

c400d23

vinferrer and others added 3 commits September 18, 2020 15:40

Update phys2bids/physio_obj.py

bee4c71

Co-authored-by: Stefano Moia <[email protected]>

change assigning trigger phrase

fd115b9

solve lintin problem

0529d79

implement partial match

2b75fd4

tsalo approved these changes Feb 12, 2021

View reviewed changes

smoia reviewed Feb 16, 2021

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

smoia reviewed Feb 16, 2021

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

smoia reviewed Feb 16, 2021

View reviewed changes

phys2bids/physio_obj.py Outdated Show resolved Hide resolved

Propose less elegant but more specific comparison

d305f4d

vinferrer and others added 13 commits February 16, 2021 12:19

Update phys2bids/physio_obj.py

99b6b3f

Co-authored-by: Stefano Moia <[email protected]>

Update phys2bids/physio_obj.py

a001a00

Co-authored-by: Stefano Moia <[email protected]>

Update phys2bids/physio_obj.py

408c63d

Co-authored-by: Stefano Moia <[email protected]>

correct linting problem

7479635

Wrong way to get an int from a string!

d8169be

Change warning to info

82f2175

Make linter happy

ce4e813

increasing logger level to info

504dca4

Merge branch 'master' of https://github.com/physiopy/phys2bids into a…

cef1363

…uto_chtrig

add linting extra line

1511054

Merge branch 'auto_chtrig' of https://github.com/vinferrer/phys2bids …

a187f06

…into auto_chtrig

Add a few trigger names to tests

52fa097

Match numbers

7e79130

smoia mentioned this pull request Feb 17, 2021

Propose less elegant but more specific comparison vinferrer/phys2bids#8

Merged

18 tasks

Merge pull request #8 from smoia/auto_chtrig

2046244

Propose less elegant but more specific comparison

smoia approved these changes Feb 17, 2021

View reviewed changes

tsalo approved these changes Feb 17, 2021

View reviewed changes

smoia self-assigned this Feb 17, 2021

smoia merged commit f6f5191 into physiopy:master Feb 17, 2021

smoia added the released This issue/pull request has been released. label Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-detect the trigger channel based on retrieved channel names #306

Auto-detect the trigger channel based on retrieved channel names #306

vinferrer commented Sep 18, 2020

vinferrer commented Sep 18, 2020

tsalo commented Sep 18, 2020

smoia left a comment

eurunuela commented Sep 18, 2020

codecov bot commented Sep 18, 2020 •

edited

Loading

vinferrer commented Sep 21, 2020

vinferrer commented Sep 21, 2020 •

edited

Loading

eurunuela commented Sep 21, 2020

tsalo commented Feb 4, 2021

vinferrer commented Feb 4, 2021

tsalo left a comment

smoia commented Feb 16, 2021 •

edited

Loading

vinferrer commented Feb 17, 2021 •

edited

Loading

smoia left a comment

tsalo left a comment

smoia commented Feb 17, 2021

Auto-detect the trigger channel based on retrieved channel names #306

Auto-detect the trigger channel based on retrieved channel names #306

Conversation

vinferrer commented Sep 18, 2020

Proposed Changes

vinferrer commented Sep 18, 2020

tsalo commented Sep 18, 2020

smoia left a comment

Choose a reason for hiding this comment

eurunuela commented Sep 18, 2020

codecov bot commented Sep 18, 2020 • edited Loading

Codecov Report

vinferrer commented Sep 21, 2020

vinferrer commented Sep 21, 2020 • edited Loading

eurunuela commented Sep 21, 2020

tsalo commented Feb 4, 2021

vinferrer commented Feb 4, 2021

tsalo left a comment

Choose a reason for hiding this comment

smoia commented Feb 16, 2021 • edited Loading

vinferrer commented Feb 17, 2021 • edited Loading

smoia left a comment

Choose a reason for hiding this comment

tsalo left a comment

Choose a reason for hiding this comment

smoia commented Feb 17, 2021

codecov bot commented Sep 18, 2020 •

edited

Loading

vinferrer commented Sep 21, 2020 •

edited

Loading

smoia commented Feb 16, 2021 •

edited

Loading

vinferrer commented Feb 17, 2021 •

edited

Loading