-
Notifications
You must be signed in to change notification settings - Fork 88
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
Adding desc keyword to BIDSPath and get_entities_from_fname? #449
Comments
Let's wait a bit. At least until there is a validator PR that is merged, I wouldn't rush on this :) |
yup, I second that |
I wanted to see if you two might have a different opinion now? Since def _add_desc_to_bids_fname(bids_fname, description, verbose: bool = True):
bids_fname, ext = _parse_ext(bids_fname, verbose)
# split by the datatype
datatype = bids_fname.split("_")[-1]
source_bids_fname = bids_fname.split(f"_{datatype}")[0]
result_fname = source_bids_fname + f"_desc-{description}" + f"_{datatype}" + ext
return result_fname You can't use The inclusion of this new entity would be a small addition and simply adds to the entity list we currently have for those two functions. :) |
can you use the check logic to have a more permissive use of the BIDSPath
object?
… |
Hi sorry to bring this up again ... :p I would again like to see if the group thinks this is possible to add in for 0.5. I definitely don't think adding full support of derivatives is possible yet considering the specification is still evolving. The main reason I'm asking again is I have a lot of bug ridden code because I have to add Since The change proposed here would allow users to change the naming of the BIDS paths in a more efficient and simple manner. If acceptable, the PR in #554 is pretty small and lean. |
Isn't the check parameter of BIDSPath the way to go to give your more
flexibility?
… |
it's true that it's technically very simple --- but in terms of API it would add an entity that is:
point 3. triggers my next question: Why do you absolutely need |
Yes, this also caught my attention and worries me a little. I'm afraid supporting this might encourage users to just cram anything into |
I had the same impression but isn't this a problem of the derivatives specification though? |
Assuming it is an oversight, my read of the spec is that
It seems like it's only for derivatives according to https://bids-specification.readthedocs.io/en/stable/05-derivatives/01-introduction.html |
Now that bids-standard/bids-specification#645 is merged in and https://bids-specification.readthedocs.io/en/latest/99-appendices/09-entities.html resolved some of the confusion on this thread with regards to i) is Description is as c/ped from the site:
Perhaps some meta checks would help here? For example, two checks can occur: i) if ? |
Is this finally ready to add now that we have https://bids-specification.readthedocs.io/en/stable/99-appendices/09-entities.html I would propose:
|
What do you mean by |
Ah okay derivative of derivatives is a tricky issue. Then maybe none of those properties actually... |
Describe the problem
Just starting a convo: it seems that the initial Derivatives PR has been merged into the BIDS specification (https://bids-specification.readthedocs.io/en/stable/05-derivatives/01-introduction.html).
Most explicitly, they seem to add this
desc-[label]
keyword argument to differentiate different files.Describe your solution
Add
desc
keyword tomake_bids_basename
.Describe possible alternatives
A separate function called
make_derivatives_basename
?Additional context
I'm wondering if there are other ways we can bottleneck how people setup their derivatives folders that adheres to the initial specification? E.g. folder naming, etc.
The text was updated successfully, but these errors were encountered: