Trigger channel index is not incremented in acq populate_phys_input

[tsalo]

Updated Issue

The acq-generated BlueprintInput objects' trigger_index attribute does not correspond to the actual trigger channel index.
The trigger index needs to be incremented down in the acq version of populate_phys_input().

Original Issue

When time is added as a channel to the time series data, the trigger channel index needs to be incremented, or else it will not reference the trigger channel. This is done in the txt version of populate_phys_input, but not in the the acq version.

Expected Behavior

The BlueprintInput object created from reading in a .acq file should have a .trigger_idx attribute pointing to the trigger channel.

Actual Behavior

BlueprintInput objects initialized from .acq files have .trigger_idx attributes pointing to the channel right before the trigger channel.

Steps to Reproduce the Problem

1. Read an .acq file.
2. Compare the chtrig argument from the populate_phys_input() call to the .trigger_idx attribute of the generated BlueprintInput object. It should have a value of chtrig + 1, not chtrig.

Specifications

- Python version: 3.7
- phys2bids version: current master at a2493bf
- Platform: OSX

Possible solution

Add 1 to chtrig when initializing the BlueprintInput object in the acq version of populate_phys_input.

[smoia]

Uh, this is indeed a bug. However, I'm not totally sure the proposed solution is the one we should be following.

In fact, at a certain point we decided to switch to channel numbering starting from 1 to be human friendly, rather than from 0 (see here. We need to change the help description (it's wrong) and check that actually the attributes are ok.

[tsalo]

To clarify, would that mean that acq-generated objects have channels starting at 1 and txt-generated ones have channels starting at 0?

[eurunuela]

They should all start at 1.

[smoia]

@eurunuela do the tests (integration) work with that in mind?

[eurunuela]

@eurunuela do the tests (integration) work with that in mind?

They should. That change was added some months ago already and I'm pretty sure @vinferrer updated the tests.

[smoia]

So in theory the only thing we need to correct (or see it is correct) are the docstrings and the CLI, right?

[eurunuela]

So in theory the only thing we need to correct (or see it is correct) are the docstrings and the CLI, right?

Sounds like that's right.

[tsalo]

The trigger index needs to be incremented down in the acq version of populate_phys_input() too, though, right?

[smoia]

@tsalo I very honestly don't remember 🙈 . Let me try it out.

[tsalo]

@smoia Thanks! I am guessing it's the case because, when I opened the issue it was because the trigger index in the Physio object didn't match the trigger channel in the time series attribute. I ended up adjusting for this in my multi-run workflow here.

[smoia]

@tsalo I hope that the bugfix I opened fixes it.

[tsalo]

Thank you @smoia. I've updated the post at the beginning of the thread to reflect the actual bug. The issue title is still mostly accurate, so I'm going to leave it for now.