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

Correct trigger channel indexing while reading AcqKnowledge files. #275

Merged
merged 7 commits into from
Jul 16, 2020

Conversation

smoia
Copy link
Member

@smoia smoia commented Jul 7, 2020

Closes #272, although the fixed problem is not the same - still, thanks @tsalo!

Turns out the algorithm was meant to extract the channel after the one of the trigger since we modified phys2bids to have -chtrig human-friendly (rather than pythonic).

Proposed Changes

  • the chtrig index is lowered by one in place when extracting the timeseries from the trigger channel.
  • the docstring now indicate that chtrig is meant to represent an index starting from 1.

@smoia smoia added the BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) label Jul 7, 2020
@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #275 into master will decrease coverage by 0.10%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
- Coverage   93.61%   93.51%   -0.11%     
==========================================
  Files          10       10              
  Lines         861      863       +2     
==========================================
+ Hits          806      807       +1     
- Misses         55       56       +1     
Impacted Files Coverage Δ
phys2bids/cli/run.py 96.55% <ø> (ø)
phys2bids/phys2bids.py 86.06% <50.00%> (-0.45%) ⬇️
phys2bids/interfaces/acq.py 100.00% <100.00%> (ø)

@tsalo
Copy link
Member

tsalo commented Jul 7, 2020

The CLI's docstring and default value for chtrig also indicate zero is the first channel. I would update those as well as add a check for either the parser's chtrig parameter or at the beginning of the phys2bids Python function to ensure that the chtrig value is an integer greater than or equal to 1.

phys2bids/interfaces/acq.py Outdated Show resolved Hide resolved
Stefano Moia and others added 5 commits July 7, 2020 22:05
phys2bids/phys2bids.py Outdated Show resolved Hide resolved
Co-authored-by: Taylor Salo <tsalo006@fiu.edu>
@vinferrer
Copy link
Collaborator

vinferrer commented Jul 16, 2020

LGTM!!! althought remmeber bugs PR only need one reviewer, you can merge already

@tsalo
Copy link
Member

tsalo commented Jul 16, 2020

@vinferrer I didn't realize that the physiopy community classified PRs like that, but that's awesome! In the docs, though, it looks like two reviews are currently required for bug fixes. Perhaps that needs to be updated?

  1. Have the necessary amount of approving reviews, even if you’re a long time contributor. You can ask one (or more) contributor to do that review, if you think they align more with the content of your PR. You need one review for documentation, tests, and small changes, and two reviews for bugs, refactoring and enhancements.

@smoia smoia merged commit b8ee56c into physiopy:master Jul 16, 2020
@smoia smoia deleted the fix/acq_chtrig branch October 7, 2020 16:54
@smoia smoia added released This issue/pull request has been released. BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) and removed BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) labels Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trigger channel index is not incremented in acq populate_phys_input
3 participants