Add ASL fields to config #282
Conversation
| subprocess.run(["mkdir", str(bids_dir / "code")]) | ||
| subprocess.run(["mkdir", str(bids_dir / "code" / "CuBIDS")]) |
There was a problem hiding this comment.
@mattcieslak do you know why CuBIDS uses subprocess.run here instead of os.makedirs? Does Datalad not play well with os.makedirs?
There was a problem hiding this comment.
this is odd, I don't remember why it's done this way
|
I'm pretty happy with the current state, but I'm not sure how best to test my changes. I guess running on the PNC and NKI datasets would be a good test? |
|
That sounds good to me. The CI tests are fairly limited |
| # Entities that may be in the filename? | ||
| file_keys = ["task", "acquisition", "direction", "reconstruction", "run"] | ||
|
|
||
| for key in file_keys: |
There was a problem hiding this comment.
@mattcieslak do you know what these entities are hardcoded here for? Are they the only entities that are used to group files? For example, if two files are the same but have different contrast agent (ce) entities, would CuBIDS treat them as part of the same key group?
There was a problem hiding this comment.
I don't think we had ever seen ce entities at this point. These were from what we'd seen while curating RBC and should definitely be expanded to better match current bids
There was a problem hiding this comment.
Gotcha. Does that mean I was interpreting the code correctly then? I.e., that those are the entities that are used to group files?
| 2. If the pull request adds functionality, the docs should be updated. Put | ||
| your new functionality into a function with a docstring, and add the | ||
| feature to the list in README.rst. | ||
| 3. The pull request should work for Python 3.5, 3.6, 3.7 and 3.8, and for PyPy. |
There was a problem hiding this comment.
Do we want to keep supporting 3.5? Other than pybids the only important dependency is sklearn and I'm not sure how far back they go
There was a problem hiding this comment.
I'd like to push forward to 3.9+ at this point, but I'd rather tackle that in a future PR.
| NON_KEY_ENTITIES = set(["subject", "session", "extension"]) | ||
| # Multi-dimensional keys SliceTiming | ||
| # Multi-dimensional keys SliceTiming XXX: what is this line about? |
There was a problem hiding this comment.
This was going to be a list of fields that are array or list type. They would need special consideration when deciding whether they're a variant or not.
There was a problem hiding this comment.
Ah, so it's a relic from an old attempt at #281?
| # Entities that may be in the filename? | ||
| file_keys = ["task", "acquisition", "direction", "reconstruction", "run"] | ||
|
|
||
| for key in file_keys: |
There was a problem hiding this comment.
I don't think we had ever seen ce entities at this point. These were from what we'd seen while curating RBC and should definitely be expanded to better match current bids
| @@ -1510,6 +1687,12 @@ def _order_columns(df): | |||
|
|
|||
| This ensures that KeyGroup and ParamGroup are the first two columns, | |||
| FilePath is the last, and the others are sorted alphabetically. | |||
|
|
|||
| Notes | |||
There was a problem hiding this comment.
I'm open to alternative strategies for how to define these values
There was a problem hiding this comment.
Since they're only used here, we could just define them in a constant in this function. WDYT?
Closes none.
Changes proposed:
aslcontext,asllabeling, andm0scanfiles.