Skip to content

ENH: Added extension to the bids.json to get the 'extension' entity#405

Closed
yarikoptic wants to merge 1 commit intobids-standard:masterfrom
yarikoptic:enh-extension
Closed

ENH: Added extension to the bids.json to get the 'extension' entity#405
yarikoptic wants to merge 1 commit intobids-standard:masterfrom
yarikoptic:enh-extension

Conversation

@yarikoptic
Copy link
Copy Markdown
Collaborator

@yarikoptic yarikoptic commented Feb 27, 2019

ATM just a proof of concept to fix #404

TODOs:

  • finish up changes to bids.json
  • fix tests
  • deprecate custom extensions arg where applicable (e.g. get)

"sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.nii.gz",
"sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}][_mod-{modality}]_{suffix<defacemask>}.nii.gz",
"sub-{subject}[/ses-{session}]/func/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_rec-{reconstruction}][_run-{run}][_echo-{echo}]_{suffix<bold>}.nii.gz",
"sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii.gz>}",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is now you have to provide the extension. What if you did:

Suggested change
"sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii.gz>}",
"sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.nii.gz",
"sub-{subject}[/ses-{session}]/anat/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio>}.{extension<nii|nii.gz>}",

},
{
"name": "extension",
"pattern": "\\.((?:[^_/\\\\]+)+\\.?)$"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only "second" extension we permit is .gz, and all extensions are alphabetical, at least for now. I'm not sure if things like surf.gii will be considered extensions, or just gii

Suggested change
"pattern": "\\.((?:[^_/\\\\]+)+\\.?)$"
"pattern": "\\.((?:[:Alpha:]+)(?:\\.gz)?)$"

Haven't tested this regex thoroughly...

@tyarkoni
Copy link
Copy Markdown
Collaborator

Probably best to hold off on this—I've got a branch going rewriting everything to use a DB. I'll probably rework the extension handling as I go through with that, but let's leave this open in case I forget.

@yarikoptic
Copy link
Copy Markdown
Collaborator Author

up to you @tyarkoni . If you decide otherwise (so that db etc could be checked with this change in mind) - let me know and I will try to get back to it to address valuable suggestions from @effigies

@tyarkoni
Copy link
Copy Markdown
Collaborator

@yarikoptic the 0.9 branch drops extensions and treats extension like any other entity. I haven't addressed the default_path_patterns yet though, or Chris's other comments above. So feel free to patch the sqlalchemy branch if you like, or you can wait till it's merged (or I can do it if you prefer).

@yarikoptic
Copy link
Copy Markdown
Collaborator Author

yarikoptic commented Apr 20, 2019

Thank you Tal!

@effigies
Copy link
Copy Markdown
Collaborator

effigies commented Sep 9, 2019

@yarikoptic I'm not sure if this has been made redundant by 0.9.x, or if there are still changes in here that you'd like to see merged. Needs a merge/rebase if so.

@tyarkoni
Copy link
Copy Markdown
Collaborator

tyarkoni commented Sep 9, 2019

I think everything proposed here was added via 0.9 or other PRs, so I think we're good to close. But @yarikoptic can confirm.

@yarikoptic
Copy link
Copy Markdown
Collaborator Author

I can no longer tell ;) let's just close it and the issue will raise from the ashes if I sense that smth like this is still needed ;-)

@yarikoptic yarikoptic closed this Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Formalize the "extension" entity to replace "extensions" argument to get

3 participants