Skip to content

[ENH] Add BIDS URIs and deprecate relative paths, RawSources and (possibly unused) BasedOn#918

Merged
sappelhoff merged 20 commits intobids-standard:masterfrom
effigies:enh/bids_uri
Jul 23, 2022
Merged

[ENH] Add BIDS URIs and deprecate relative paths, RawSources and (possibly unused) BasedOn#918
sappelhoff merged 20 commits intobids-standard:masterfrom
effigies:enh/bids_uri

Conversation

@effigies
Copy link
Copy Markdown
Collaborator

@effigies effigies commented Oct 29, 2021 •
edited by sappelhoff
Loading

This PR replaces #820, which I'm leaving open for comparison purposes.

I have moved BIDS URI as a sub-section of URI, and drastically reduced the amount of text, including justification and examples. I have also reduced the scope to purely include files in BIDS datasets, as standard URIs are sufficient for other resources.

I am happy to consider re-adding the justification and some examples to an appendix, but I think keeping Common Principles lean is a good idea.

One significant semantic change: Reading up on file:/// URLs, there is no scope for these to be relative. Relative paths are simply that, and are resolved based on context. I have thus permitted DatasetLinks to include derivatives/xyz as a valid location.

closes #790
closes #471
closes #718
#757 (or at least, pave the way for it to be closed)

@effigies effigies mentioned this pull request Oct 29, 2021
Closed
satra
satra reviewed Oct 30, 2021
View reviewed changes
sappelhoff
sappelhoff reviewed Nov 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

thanks for narrowing the scope of this and writing it up so clearly. I am gonna do a double check of all fields that would need to be adjusted - but perhaps you already caught them all.

Some comments meanwhile.

@sappelhoff sappelhoff mentioned this pull request Nov 8, 2021
Closed
sappelhoff
sappelhoff reviewed Nov 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

I think the BasedOn field is the only one that remains to be adjusted (right now).

Taken from my list in #820 ... Some of the items I had on there were invalid, one of them is now deprecated, and the rest should be in this PR - and only BasedOn is missing as far as I can see.

  • IntendedFor
  • SpatialReference (invalid)
  • AssociatedEmptyRoom
  • B0FieldIdentifier (invalid)
  • B0FieldSource (invalid)
  • BasedOn
  • RawSources (deprecated)
  • Sources

@effigies
Copy link
Copy Markdown
Collaborator Author

effigies commented Nov 8, 2021

Should BasedOn be Sources? It's only specified in an appendix as a qMRI-specific derivative metadata field, and seems nearly identical to RawSources (which will be merged into Sources).

cc @agahkarakuzu @Gilles86 for input.

@sappelhoff sappelhoff mentioned this pull request Nov 8, 2021
Merged
@agahkarakuzu
Copy link
Copy Markdown
Contributor

agahkarakuzu commented Nov 8, 2021

Thanks @effigies ! BasedOn may be a field introduced early in BEP001, then not adapted to following conventions. As you noted, RawSources (or Sources after the merge) is identical. I am OK for the modifications this requires, i.e. changing BasedOn --> Sources in the qMRI appendix. Example datasets will need to be updated as well.

@effigies
Copy link
Copy Markdown
Collaborator Author

effigies commented Nov 9, 2021

Thanks, Agah! @sappelhoff I guess technically we should probably deprecate BasedOn like we will RawSources?

@sappelhoff
Copy link
Copy Markdown
Member

sappelhoff commented Nov 9, 2021 •
edited
Loading

Thanks, Agah! @sappelhoff I guess technically we should probably deprecate BasedOn like we will RawSources?

Sounds good! Too bad we didn't catch this earlier - but better late than never.

PS: Re: adjusting examples --> running grep -irn "BasedOn" . on bids-examples does not yield anything. So perhaps this field was never used?

@effigies effigies changed the title ENH: Add BIDS URIs, deprecate relative paths ENH: Add BIDS URIs and deprecate relative paths, RawSources and (possibly unused) BasedOn Nov 9, 2021
@sappelhoff sappelhoff added this to the 1.7.0 milestone Nov 9, 2021
@nicholst
Copy link
Copy Markdown
Collaborator

nicholst commented Nov 10, 2021

I've reviewed this at a high level, and have no comments except to say that issues that I caught in #820 have been addressed here.

@effigies
Copy link
Copy Markdown
Collaborator Author

effigies commented Nov 10, 2021

@sappelhoff I guess for something this big, we should probably patch the validator and update some examples to ensure that somebody following these rules won't be unable to validate.

@Remi-Gau Remi-Gau mentioned this pull request Nov 11, 2021
Merged
6 tasks
@sappelhoff
Copy link
Copy Markdown
Member

sappelhoff commented Nov 11, 2021

@sappelhoff I guess for something this big, we should probably patch the validator and update some examples to ensure that somebody following these rules won't be unable to validate.

agreed 👍 we should have a PR ready for the validator to merge once this is merged/released ... and updating the examples is a good idea as well.

@VisLab
Copy link
Copy Markdown
Member

VisLab commented Nov 19, 2021

I have gone over the changes and support this pull request.

@effigies
Squashed commit of the following:
88fe87b 
commit 4fef34d
Author: Christopher J. Markiewicz <markiewicz@stanford.edu>
Date:   Fri Oct 29 11:50:22 2021 -0400

    RF: Narrow scope and reorganize BIDS URI section

commit 7f04af7
Merge: 704bedc f21e9b2
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Thu Sep 2 17:29:48 2021 +0200

    Merge branch 'master' into enh/point_to_data

commit 704bedc
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Thu Aug 12 10:36:25 2021 +0200

    address wording suggested by Kay

commit 0c21ae5
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Wed Aug 11 11:40:29 2021 +0200

    clarify derivatives DATASET versus FOLDER

commit 554e1e1
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Wed Aug 11 11:30:57 2021 +0200

    extend examples, docs, and recommendations for REMOTE bids uris

commit 2d49121
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Wed Aug 11 10:03:03 2021 +0200

    change order: within, remote, outside -> within, outside, remote

commit 55f988d
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Aug 10 13:31:36 2021 +0200

    improve 'outside of ds' example

commit 475b4ca
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Aug 10 12:12:57 2021 +0200

    deriv3 -> deriv4 for 4th example

commit b82f7ad
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Aug 10 11:56:40 2021 +0200

    consistently refer to data as dataset where appropriate

commit 8bdd55d
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 14:34:30 2021 +0200

    semantic line breaks and add RECOMMENDATIONS

commit 1a7d870
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 13:59:53 2021 +0200

    fix link syntax

commit fe547c9
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 13:50:44 2021 +0200

    fix schema links

commit 8a36aa2
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 13:30:39 2021 +0200

    fix Windows spec to contain a colon: /C/... --> /C:/...

commit 3acd887
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 13:25:04 2021 +0200

    change BIDS URI reserved keyword: bids:local:/... -> bids::/...

commit 742db02
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 13:15:59 2021 +0200

    make spatialReference example URI based again (not BIDS URI)

commit 42dd6cd
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 12:24:30 2021 +0200

    misc fixes and additions

commit 91e332d
Merge: 4b0a18f 9a79dff
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Aug 9 11:35:42 2021 +0200

    Merge branch 'master' into enh/point_to_data

commit 4b0a18f
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Jun 28 14:21:41 2021 +0200

    improve brainplot example

commit 955f78f
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Jun 28 14:12:22 2021 +0200

    add info on what to do with nested derivatives dirs

commit 5ba3b6e
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Jun 28 13:58:45 2021 +0200

    clarify derivatives spatialreferences

commit 29db468
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Jun 28 13:55:21 2021 +0200

    update qmri BasedOn to use BIDS URIs

commit 79b7e84
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Jun 28 13:50:11 2021 +0200

    adjust Sources and RawSources to use BIDS URIs

commit 7760f02
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Jun 28 13:44:26 2021 +0200

    complete SpatialReferences example

commit 78de170
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Wed Jun 23 22:02:13 2021 +0200

    fix: add missing word

commit 4a87df8
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Mon Jun 14 11:49:30 2021 +0200

    address misc feedback on wording by Kay.

commit dd08ced
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Fri Jun 11 10:03:01 2021 +0200

    add missing newline

commit 6263793
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Fri Jun 11 10:01:42 2021 +0200

    clarify local paths: WINDOWS, UNIX

commit e62ee75
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Fri Jun 11 09:49:31 2021 +0200

    add warning about non-portable BIDS URIs

commit 1872fe7
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Fri Jun 11 09:44:39 2021 +0200

    clarify REMOTE example, RECOMMEND DOI over arbitrary servers

commit e3d094b
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Fri Jun 11 09:28:29 2021 +0200

    reorder example sections from easy to hard

commit 68821ec
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Fri Jun 11 09:26:33 2021 +0200

    clarify local references WITHIN ds

commit 2385745
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Fri Jun 11 09:14:33 2021 +0200

    deprecate at 1.7.0, not 1.6.1

commit 49b6662
Merge: 430429d 17b5444
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 20:48:38 2021 +0200

    Merge branch 'master' into enh/point_to_data

commit 430429d
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 14:23:16 2021 +0200

    links

commit 9e72f28
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 14:18:09 2021 +0200

    more links and typos

commit 97176ef
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 14:10:03 2021 +0200

    fix more links

commit 8d799b4
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 14:07:05 2021 +0200

    fix links

commit 2c80316
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 14:04:56 2021 +0200

    pacify linter, fix build

commit 4446df7
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 13:57:00 2021 +0200

    fix typos

commit 0f3a278
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 13:49:58 2021 +0200

    overhaul AssociatedEmptyroom fields across the spec

commit f2f502e
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 13:47:35 2021 +0200

    overhaul SpatialReference fields across the spec

commit 60b437f
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 13:44:41 2021 +0200

    overhaul IntendedFor fields across the spec

commit 80a6011
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 13:28:49 2021 +0200

    add DatasetLinks field to dataset_description.json docs

commit b320026
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 13:21:08 2021 +0200

    add examples for within, outside, remote

commit a159e53
Author: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Date:   Tue Jun 8 11:33:16 2021 +0200

    add reasoning on why relative path is deprecated
@effigies
RF: Refer to dataset-relative paths as relative paths, drop leading s…
b84f935 
…lash
@effigies
ENH: Mark dataset-name as optional, consistently use <relative-path>
f65fb36
@effigies
ENH: Clarify sheme-less URIs in DatasetLinks
6ef0073
@effigies
FIX: Direct readers to resolution section for local URIs
a311eac
@effigies
FIX: Use relative paths in all BIDS URIs
f383385
@effigies
ENH: Qualify datasets as BIDS datasets when not absolutely clear
300771b
@effigies
RF: Deprecate BasedOn, recommend Sources + BIDS URI
d9733f0
@effigies effigies mentioned this pull request Dec 7, 2021
Closed
2 tasks
@sappelhoff sappelhoff added this to the 1.8.0 milestone Jul 13, 2022
@sappelhoff sappelhoff mentioned this pull request Jul 21, 2022
Merged
@sappelhoff sappelhoff requested a review from erdalkaraca as a code owner July 21, 2022 17:27
@sappelhoff sappelhoff requested a review from ezemikulan as a code owner July 21, 2022 17:27
@sappelhoff sappelhoff changed the title ENH: Add BIDS URIs and deprecate relative paths, RawSources and (possibly unused) BasedOn [ENH] Add BIDS URIs and deprecate relative paths, RawSources and (possibly unused) BasedOn Jul 21, 2022
sappelhoff
sappelhoff reviewed Jul 22, 2022
View reviewed changes
@effigies @sappelhoff
Update src/05-derivatives/02-common-data-types.md
7cb10e0 
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
effigies
effigies commented Jul 22, 2022
View reviewed changes
sappelhoff
sappelhoff reviewed Jul 23, 2022
View reviewed changes
sappelhoff
sappelhoff approved these changes Jul 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

apart from my code suggestion on leading slashes (based on bids-standard/bids-examples#304 (comment)), I think this can finally go in

@effigies @sappelhoff
Update src/02-common-principles.md
13e5f71 
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@sappelhoff sappelhoff merged commit 6e2a0b0 into bids-standard:master Jul 23, 2022
bids-maintenance added a commit that referenced this pull request Jul 23, 2022
@bids-maintenance
[DOC] Auto-generate changelog entry for PR #918
a3e5df2
This was referenced Jul 25, 2022
Merged
Merged
Open
Merged
Closed
Draft
Open
@yarikoptic yarikoptic mentioned this pull request Jul 25, 2022
Closed
@VisLab VisLab mentioned this pull request Jul 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@satra satra satra left review comments

@sappelhoff sappelhoff sappelhoff approved these changes

@chrisgorgo chrisgorgo Awaiting requested review from chrisgorgo

@tsalo tsalo Awaiting requested review from tsalo

@erdalkaraca erdalkaraca Awaiting requested review from erdalkaraca erdalkaraca is a code owner

@ezemikulan ezemikulan Awaiting requested review from ezemikulan

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

1.8.0

6 participants

@effigies @agahkarakuzu @sappelhoff @nicholst @VisLab @satra