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

Common principles: More elaborate "entity" definition #4

Merged
merged 3 commits into from
Apr 29, 2022

Conversation

Lestropie
Copy link
Owner

Relates to bids-standard#947.

This is an attempt at the proposal I made in this comment. There is however a little bit of a modification. As shown in the poll, there is no consensus around exactly what it is that is referred to as an "entity". In my comment, I had suggested that "entity" refers to a higher-order concept, of which things like the "name" (e.g. "reconstruction") and the "key" (e.g. "rec") are attributes. I had proposed that any and all references to such attributes would need to be explicit regarding exactly to what was being referred. However I found this to be quite clumsy. The proposal here therefore includes the following comment:

Depending on context, any one of the entity name, key, format, or a specific
entity instance, may in some instances be referred to as simply an "entity".

This means that most of the proposed changes in bids-standard#947 relating to the separation between a key-value entry in a JSON and an entity in a file name remain unchanged; this particular PR only addresses the definition of "entity".


Note that, rather than simply including "entity" in the list of definitions at the commencement of "common principles", I have instead given it its own Level 2 heading. This is such a core concept to BIDS that I believe it warrants greater precedence than other terms that necessitate definition; moreover, these nuances about the various attributes of entities and the way they are described requires a greater volume of text than is required for the other defined terms. There are some surrounding changes necessary because the dictionary terms are defined before the "Entity" heading; happy to separate these out if maintainers see fit, this is just the initial set of changes I deemed necessary to construct the definition as desired.

Copy link

@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 this very clear PR @Lestropie -- I think your added definitions of different aspects of an entity improve the situation, and being lenient with what to call "an entity" (the instance, the key, ...) will probably work well with the situation you identified with your poll in bids-standard#947

I left a few minor comments.
One additional comment: Could you please insert line breaks after the end of each sentence, and meaningfully after "semantic break points" in a sentence?
That will probably help us to get a cleaner git diff if some particular sentence needs to be changed down the line.

src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
Comment on lines 190 to 192
label for each session. In datasets where at least one subject has more than
one session, this additional sub-directory later SHOULD be added for all
subjects in the dataset. Additional information on each session MAY be

Choose a reason for hiding this comment

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

I know this recommendation was in the spec prior to this PR, but I don't really get the point: What if subjects miss some sessions ... or if it's a between subject design with different subjects being part of different sessions?

See also the recommendation below:

A data type directory SHOULD NOT be defined if
there are no files to be placed in that directory.

We don't have to discuss it here, but if someone has a quick point on this, I am happy to hear it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

My suspicion is that it was intended to make things easier for App developers. If a hypothetical App is hard-wired to loop over multiple sessions within a subject and process each individually, but without adequate logic to detect the absence of a session structure in any particular subject, passing such a dataset could break the App. Personally I'm happy to have them break; but I'm a known software sadist so that might not count for much.

Lestropie added a commit that referenced this pull request Apr 26, 2022
Co-authored by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Requests made in: #4.
Lestropie added a commit that referenced this pull request Apr 26, 2022
Lestropie added a commit that referenced this pull request Apr 26, 2022
Lestropie added a commit that referenced this pull request Apr 26, 2022
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Requests made in: #4.
Note that this is a repeat of commit 3972dd7, but with corrected multi-author attribution.
Requests made in: #4.
Note that this is a repeat of commit 3972dd7, but with corrected multi-author attribution.

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@Lestropie Lestropie merged commit f3df02b into define_entity Apr 29, 2022
@Lestropie Lestropie deleted the entity_refined_definition branch April 29, 2022 00:33
sappelhoff added a commit to bids-standard/bids-specification that referenced this pull request Jun 8, 2022
* [FIX] Common principles: Add "entity" to list of definitions

* Common principles: Link "entity" definition to appendices

* Standardise use of "entity" definition across specification

* Common principles: More elaborate "entity" definition

* Common principles: Reformmating of Entity definition changes

Requests made in: Lestropie#4.
Note that this is a repeat of commit 3972dd7, but with corrected multi-author attribution.

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

* Common principles: Remark fixes

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants