Conversation
|
Wait, we have to get rid of Travis. I'll open an issue. |
tsalo
left a comment
tsalo
left a comment
There was a problem hiding this comment.
I may be misunderstanding the required inputs for the workflow. I was under the impression that the workflow should be designed around BIDS-format data. If not, then only some of these comments will apply.
phys2denoise/cli/run.py
Outdated
| optional.add_argument('-os', '--oversampling', | ||
| dest='oversampling', | ||
| type=int, | ||
| help='Temporal oversampling factor in seconds. ' |
There was a problem hiding this comment.
I don't think the oversampling factor has a unit. It's just a scalar for the actual sample rate.
There was a problem hiding this comment.
I just took that from the input descriptions in PR #4 but I have no idea
phys2denoise/cli/run.py
Outdated
| optional.add_argument('-tl', '--time-length', | ||
| dest='time_length', | ||
| type=int, | ||
| help='RRF Kernel length in seconds.', |
There was a problem hiding this comment.
Does this apply to the CRF kernel as well?
There was a problem hiding this comment.
I think so, but as I said I based these inputs in PR #4
smoia
left a comment
smoia
left a comment
There was a problem hiding this comment.
Ok, a couple of changes here and there but there's no gain without pain, right?
phys2denoise/cli/run.py
Outdated
| help='Sampling rate for the output time series ' | ||
| 'in seconds. Corresponds to TR in fMRI data.', |
There was a problem hiding this comment.
We could rename the variable in the metrics that use this one
phys2denoise/cli/run.py
Outdated
| optional.add_argument('-slt', '--slice-timings', | ||
| dest='slice_timings', | ||
| type=str, | ||
| help='Filename with the slice timings.', | ||
| default=None) |
There was a problem hiding this comment.
Yes, but atm either we import the fMRI file, or we need to get the information in another way - let's do the other way to begin with.
Co-authored-by: Stefano Moia <s.moia@bcbl.eu>
|
Hey, I made all the changes mentioned in your comments except the one regarding the nscans because l'm not sure what it is exactly. |
smoia
left a comment
smoia
left a comment
There was a problem hiding this comment.
Some suggested changes to take care of, but I think it's fine. @tsalo @CesarCaballeroGaudes does it make sense to you?
4 participants
First draft of the parser.
I'm particularly not sure about slice_timing and n_harmonies.
Also pardon me for some poor descriptions in help.
I'm waiting for your comments!