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] Add BIDS URIs and deprecate relative paths, RawSources and (possibly unused) BasedOn #918

Merged
merged 20 commits into from
Jul 23, 2022

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Oct 29, 2021

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)

src/02-common-principles.md Outdated Show resolved Hide resolved
Copy link
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.

src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
Copy link
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
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.

@agahkarakuzu
Copy link
Contributor

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
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
Member

sappelhoff commented Nov 9, 2021

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
Collaborator

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
Collaborator Author

@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.

@sappelhoff
Copy link
Member

@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
Member

VisLab commented Nov 19, 2021

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

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
@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
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Copy link
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

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants