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

first draft of general conventions page #26

Merged
merged 18 commits into from
Nov 27, 2023

Conversation

PeerHerholz
Copy link
Member

@PeerHerholz PeerHerholz commented Oct 19, 2023

This PR and the respective commits aim to introduce a first draft of a new page/section concerning general conventions for BEP development. This was discussed here and addresses this issue.

To this end, a new page called "General conventions" is introduced within/from which specific conventions are included/linked. In the current form, parts of the original discussion/proposal were copy-pasted and adapted within a section called "general conventions for spatial derivatives".

Adding @dlevitas to this endeavor.

This PR and the respective commits aim to introduce a first draft of a new page/section concerning  general conventions for BEP development. This was discussed [here](bids-standard/bids-specification#1602) and addresses [this issue](bids-standard#24). 

To this end, a new page called "General conventions" is introduced within/from which specific conventions are included/linked. 
In the current form, parts of the original discussion/proposal were copy-pasted and adapted within a section called "general conventions for spatial derivatives".
@PeerHerholz
Copy link
Member Author

Hi folks,

I just wanted to ask if someone had any thoughts re this (@bids-standard/steering, @bids-standard/maintainers)?

Thanks again.

Cheers, Peer

Copy link
Contributor

@yarikoptic yarikoptic 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 it is worth

  • unify to "guidelines" (renaming of the file is due to then)
  • separate out "general" from "BEP specific" (or come up with some other term) guidelines. I gave examples of a few generic guidelines, we could add more.

docs/general_conventions.md Outdated Show resolved Hide resolved

Based on the evergrowing set of `BEP`s and the respective work and efforts conducted to develop them, the community has identified a set of general conventions that can be used to guide the corresponding processes. These conventions are not part of the [BIDS specification](https://bids-specification.readthedocs.io/en/stable/index.html), but rather a set of guidelines that are recommended to be followed when developing a `BEP`. These conventions are not set in stone and can be modified as needed. However, we recommend that any changes to these conventions be discussed with the `BIDS community` before being implemented. The guidelines are thus RECOMMENDED, not REQUIRED, in that `BEP`s would be allowed to deviate when deemed necessary. The goal is to establish consensus so that parts of `BEP`s that propose terms in line with guidelines will be considered accepted in principle.

## General conventions for spatial derivatives
Copy link
Contributor

Choose a reason for hiding this comment

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

By General conventions I envision conventions General to all BEPs, and that is what suggested in opening paragraph. Here it seems that conventions are already more specific, and thus not General. Let's place them into a separate section?

Suggested change
## General conventions for spatial derivatives
# BEP specific guidelines
Sometimes multiple BEPs touch upon an aspect which is relevant to all of them.
## Guidelines for spatial derivatives

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC the idea was to work on and formalize conventions that would apply across multiple BEPs based on their respective modality and/or included analyses. Thus, the structure would be: general conventions independent of modality/analyses -> modality/analyses-specific conventions and therefore, there wouldn't be a BEP specific section. However, I could of course be wrong re this.

Copy link
Contributor

Choose a reason for hiding this comment

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

They're not exactly BEP-specific, but they are decisions that were triggered by the needs specific BEPs. Possibly there's a better heading, but I'm okay with separating out universal and more narrowly targeted guidelines.

docs/general_conventions.md Outdated Show resolved Hide resolved
docs/general_conventions.md Outdated Show resolved Hide resolved
@PeerHerholz
Copy link
Member Author

Hi everyone,

thx @yarikoptic for the feedback and input, that's highly appreciated!

@francopestilli, @arokem, @effigies, @oesteban: as the other authors of the initial proposal, what do you think about the proposed changes?

Cheers, Peer

@@ -0,0 +1,52 @@
# BEP general conventions

Based on the evergrowing set of `BEP`s and the respective work and efforts conducted to develop them, the community has identified a set of general conventions that can be used to guide the corresponding processes. These conventions are not part of the [BIDS specification](https://bids-specification.readthedocs.io/en/stable/index.html), but rather a set of guidelines that are recommended to be followed when developing a `BEP`. These conventions are not set in stone and can be modified as needed. However, we recommend that any changes to these conventions be discussed with the `BIDS community` before being implemented. The guidelines are thus RECOMMENDED, not REQUIRED, in that `BEP`s would be allowed to deviate when deemed necessary. The goal is to establish consensus so that parts of `BEP`s that propose terms in line with guidelines will be considered accepted in principle.
Copy link
Member

Choose a reason for hiding this comment

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

this should be reformatted to start a new line after each sentence (~semantic line breaks https://sembr.org/)

Copy link
Member

Choose a reason for hiding this comment

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

also below and throughout this document --> it will result in a cleaner git diff in the future :-)

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 Peer, I agree with the proposed additions in general, but cannot really make a statement about the spatial derivatives in particular, because I lack expertise in that domain.

@effigies
Copy link
Contributor

effigies commented Nov 6, 2023

I'm okay with the text in general, and I don't think we need to be overly fussy about wording in this repo so long as there are not glaring inconsistencies with what was decided. We can always propose clarifying updates.

I think it would be easier for me to make concrete suggestions if you accept (with modifications, if you like) Yarik's changes and add line breaks. Right now it's hard not to conflict.

@PeerHerholz
Copy link
Member Author

Thx @sappelhoff and @effigies,

I will accept the changes to streamline further feedback.

PeerHerholz and others added 3 commits November 6, 2023 15:39
Simplify intro and add line breaks as suggested by @yarikoptic.

Co-authored-by: Yaroslav Halchenko <[email protected]>
Change wording from "conventions" to "guideline" as suggest by @yarikoptic.

Co-authored-by: Yaroslav Halchenko <[email protected]>
Add section re general guidelines/recommendations as suggested by @yarikoptic.

Co-authored-by: Yaroslav Halchenko <[email protected]>
mkdocs.yml Outdated Show resolved Hide resolved
PeerHerholz and others added 2 commits November 7, 2023 11:07
rename general_conventions.md to general_guidelines.md

Co-authored-by: Yaroslav Halchenko <[email protected]>
rename general_conventions.md to general_guidelines.md as suggested by @yarikoptic.
Copy link
Contributor

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

A fast first pass addressing some stylistic issues (e.g. start new sentences with new line) and minor comments here and there.

I find the use of backticks to emphasize acronyms and words distracting.

@@ -0,0 +1,67 @@
# BEP general guidelines

Based on the evergrowing set of `BEP`s and the respective work and efforts conducted to develop them, the community has identified a set of general guidelines that can be used to guide the corresponding processes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the mkdocs accept abbreviations? Because the backticks use here feels annoying to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

(it's a nitpick anyways)


Based on the evergrowing set of `BEP`s and the respective work and efforts conducted to develop them, the community has identified a set of general guidelines that can be used to guide the corresponding processes.
These guidelines are not part of the [BIDS specification](https://bids-specification.readthedocs.io/en/stable/index.html), but rather are recommended to be followed when developing a `BEP`.
These guidelines are not set in stone and can be modified as needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment would need further elaboration w.r.t. stated rules and guidelines policing the process of modification. Otherwise, remove.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did we have some additional information re this? Also asking @yarikoptic, @arokem and @francopestilli?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is not necessary unless we have something more explicit to say here (I don't think we do)

docs/general_guidelines.md Outdated Show resolved Hide resolved
docs/general_guidelines.md Outdated Show resolved Hide resolved
## Facilitate atomic changes

See [issue #371](https://github.com/bids-standard/bids-specification/issues/371) for motivation and discussion.
It is recommended to identify perspective entities and metadata fields to be added, and research if they, or their synonyms, are already considered in submitted PRs or other BEPs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps who or how to keep track of other PRs and BEPs would be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about the BEP leads are responsible for this and the maintainers verify?

docs/general_guidelines.md Outdated Show resolved Hide resolved
docs/general_guidelines.md Outdated Show resolved Hide resolved
docs/general_guidelines.md Outdated Show resolved Hide resolved
docs/general_guidelines.md Outdated Show resolved Hide resolved
docs/general_guidelines.md Outdated Show resolved Hide resolved
@CPernet
Copy link
Contributor

CPernet commented Nov 8, 2023

sorry to jump in late but if 'These guidelines are not part of the [BIDS specification](https://bids-specification.readthedocs.io/en/stable/index.html), but rather are recommended to be followed when developing a BEP. why is it to be merged in the BIDS specification, i.e. /docs? and not merged to /bids-extensions (IMO this either goes into spec derivatives and it is part of the spec or goes to extension -- please no new section)

PeerHerholz and others added 8 commits November 8, 2023 11:42
Add line breaks and rephrasing.

Co-authored-by: Oscar Esteban <[email protected]>
Add line breaks and rephrasing.

Co-authored-by: Oscar Esteban <[email protected]>
Add line breaks.

Co-authored-by: Oscar Esteban <[email protected]>
Add line breaks.

Co-authored-by: Oscar Esteban <[email protected]>
Add line breaks.

Co-authored-by: Oscar Esteban <[email protected]>
Add line breaks.

Co-authored-by: Oscar Esteban <[email protected]>
Add line breaks.

Co-authored-by: Oscar Esteban <[email protected]>
Add line breaks.

Co-authored-by: Oscar Esteban <[email protected]>
@PeerHerholz
Copy link
Member Author

Hi @CPernet,

sorry if I did something wrong but this should merge into /bids-extensions, ie here as indicated above.

PeerHerholz and others added 3 commits November 16, 2023 10:46
This commit adds line breaks in the introduction and removes a sentence regarding diverging from the guidelines (as no respective process was discussed/provided).
@PeerHerholz
Copy link
Member Author

Hi everyone,

would the current status be ok re merging?

Cheers, Peer

Copy link
Contributor

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Apologies for the slow response. I think this is overall good. A small comment on an evolution of BEP 23 that I wasn't aware of when we wrote up the initial draft.

docs/general_guidelines.md Outdated Show resolved Hide resolved
Update to latest status of BEP23, ie `mimap`s and `stat-`mean/`std` maps as suggested by @effigies.

Co-authored-by: Chris Markiewicz <[email protected]>
@effigies effigies merged commit 6c4c65c into bids-standard:main Nov 27, 2023
arokem added a commit to arokem/bids-bep016 that referenced this pull request Mar 29, 2024
Based on recent additions to the BIDS extensions principles in
bids-standard/bids-extensions#26.

Along the way, we follow the inheritance principle by reducing
the number of metadata files in each directory to one per model,
across all of the model parameters, and eliminating the distinction
between model fit and model derived parameters.
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.

8 participants