Restructure #38
Restructure #38
Conversation
…d objects, started rewriting of main for objects
…cts, added method to find offset in time in blueprint_input.
…and check it's supported by phys2bids
|
Still working on |
…and output object population, started writing real output.
…istence, moved heuristics around, added renaming of channels using new classes, improved bids naming and output in case of multiple frequencies.
Alright! |
There was a problem hiding this comment.
Hey @smoia ! Sorry for the delay in review—this was a lot to go through!
I had lots of thoughts (sorry), but one major point I'm a bit confused on is the difference between the blueprint_input and blueprint_output classes? From what I can tell they're largely similar in functionality, so I'm not sure the duplication is necessary!
Beyond that there's lots of little questions here and there, but I think if we settle on the class functionality in physio_obj.py then we can work on making that object a bit easier to work with. But that will come later—once this is merged and we have some tests added! 👍
Great start, though. Let me know if you have any questions about my questions / comments.
phys2bids/utils.py
Outdated
| if filename[-len(ext):] != ext: | ||
| filename = filename + ext | ||
|
|
||
| return file | ||
| return filename |
There was a problem hiding this comment.
from pathlib import Path
def check_input_ext(filename, ext):
return Path(filename).with_suffix(ext)will do a lot of nice checks (e.g., confirm that ext is a valid suffix that starts with a dot, check if filename has a different suffix and strip it first). Might be worth using that instead? This will also make the below function (check_input_type()) a bit safer... See comment there.
There was a problem hiding this comment.
This will fail in the case of .nii.gz though (foreseeing application). We can use both?
There was a problem hiding this comment.
Actually this will fail in any possible .*.gz, like .tsv.gz.
I'd like a safe solution for such cases.
| Properties - Input | ||
| ------------------ | ||
| timeseries : (ch x tps) :obj:`numpy.ndarray` | ||
| Numpy 2d array of timeseries | ||
| Contains all the timeseries recorded. | ||
| Impose same frequency! | ||
| freq : float | ||
| Shared frequency of the object. | ||
| Properties | ||
| ch_name : (ch) list of strings | ||
| List of names of the channels - can be the header of the columns | ||
| in the output files. | ||
| units : (ch) list of strings | ||
| List of the units of the channels. | ||
| start_time : float | ||
| Starting time of acquisition (equivalent to first TR, | ||
| or to the opposite sign of the time offset). | ||
|
|
||
| Methods | ||
| ------- | ||
| return_index: | ||
| Returns the proper list entry of all the | ||
| properties of the object, given an index. | ||
| delete_at_index: | ||
| Returns all the proper list entry of the | ||
| properties of the object, given an index. | ||
| init_from_blueprint: | ||
| method to populate from input blueprint instead of init |
There was a problem hiding this comment.
I'm not sure I'm getting what benefits this has over blueprint_input. The main differences (from what I can tell!) are that timeseries is required to be an array instead of a list of arrays, and this doesn't have a few of the methods that the blueprint_input does (e.g., .check_trigger_amount() and .rename_channels()). Could you clarify what purpose this class is serving over-and-above blueprint_input?
There was a problem hiding this comment.
So here's the thing. For latter uses (e.g. if we go on with preprocessing or data analysis), I'd say numpy arrays are a bit easier to work with, rather than list of numpy arrays.
However, as the input might have different sampling rates, we need lists at the beginning to avoid issues with n/a or lengths.
Also, in input freq is a list and in output it's a float, and we need to really check this and be sure it's respected.
What would be a solution otherwise?
Co-Authored-By: Vicente Ferrer <[email protected]>
There was a problem hiding this comment.
I believe some thorough testing should be done. Since we still have to work on the tests, and given that the package is still not compatible with some files, I'm accepting the PR. I'm aware of the possible issues that will raise with given file formats but I count on them being fixed once the testing is ready.
…py` `physio_obj.py`.
There was a problem hiding this comment.
Agree with @eurunuela. Let's go ahead and merge this in, add some tests for the various utility functions, and re-address any bugs we find after.
Resolves #10 .
Address #23 , #1 .
After sharing some thoughts with @rmarkello , I'm reformatting the code so that we have an object to populate within interfaces scripts - each one to call based on the input.