Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

yarikoptic
Copy link
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
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
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
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
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
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
Collaborator Author

yarikoptic commented Apr 20, 2019

Thank you Tal!

@effigies
Copy link
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
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
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