-
Notifications
You must be signed in to change notification settings - Fork 44
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
Refactor io
functions related to txt file imports.
#349
Conversation
…ead it outputs its results as a funtion would do
…heck_tigger_amounts
…e the same inputs
.txt
extension functions.txt
extension functions
.txt
extension functionsio
functions related to txt file imports.
Codecov Report
@@ Coverage Diff @@
## master #349 +/- ##
==========================================
- Coverage 94.85% 94.80% -0.06%
==========================================
Files 8 8
Lines 836 808 -28
==========================================
- Hits 793 766 -27
+ Misses 43 42 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the changes. Thank you @vinferrer !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing to address (either change or reply to the suggestion), but then it's ready!
phys2bids/io.py
Outdated
elif interval[-1] == 'µs': | ||
interval[0] = float(interval[0]) / 1000000 | ||
interval[-1] = 's' | ||
elif interval[-1] == 'sec': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try again.
The last moment you use interval[-1]
in this function is during this if
statement, to see if the value of interval[0]
should be "corrected". After this if
statement, interval[-1]
becomes unused - from what I see, you're adding the unit s
in the unit
list in the hard way (coherently with your script).
For this reason, every interval[-1]='s'
in this elif
statement can be removed (unless you're keeping it for testing, but then please write it).
Lines 152-154 are actually superfluous. Line 152 and 154 do exactly what phys2bids.bids.bidsify_unit()
does downstream, and line 153 is the same as line 156. The only change you would have to do (that is not wrong) is adding the option in line 131 for interval[-1] != 's' or interval[-1] != 'sec'
. Which also makes sense for the message you're giving after in 132. It would be weird, if I know that I set up my labchart to record my sampling interval in seconds, to see that the sampling interval is not second.
You can save almost ten lines, I would do it!
Okay, now I understand. It's done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last little suggestion, then it's going to be good on my side.
Co-authored-by: Stefano Moia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Great job!
(and yeah, we should find a way to avoid codeconv to protest every time) |
@eurunuela if you're happy you can merge. |
Thank you @vinferrer ! |
🚀 PR was released in |
Closes nothing
Proposed Changes
io.py
process_acq
andprocess_txt
intoprocess_blueprint_items
extract_header_items
function that gets the header and extractsinterval, orig_units, orig_names
and gives them toprocess_blueprint_items
Change Type
bugfix
(+0.0.1)minor
(+0.1.0)major
(+1.0.0)refactoring
(no version update)test
(no version update)infrastructure
(no version update)documentation
(no version update)other
Checklist before review