search

bids-standard / bids-specification

Brain Imaging Data Structure (BIDS) Specification
https://bids-specification.readthedocs.io/
Creative Commons Attribution 4.0 International
275 stars 157 forks source link

Suggestion: (Partially) Decompose BEP-001 #512

Closed effigies closed 3 years ago

effigies commented 4 years ago

Would it be possible to separate out the new entities into their own PRs? It seems like the new entities (part, fa, inv, and mt) are fairly self-contained and could be reviewed in distinct chunks.

Originally posted by @tsalo in https://github.com/bids-standard/bids-specification/pull/508#issuecomment-648199932

I didn't want to start a new conversation on #508 to keep things focused. But this is worth discussing. To do this, we would need consent from the BEP-001 team, as this would cause some disruption, and (if they consent) to determine who will do the work of separating the PRs and keeping branches synchronized (does not need to be part of the existing BEP-001 team).

effigies commented 4 years ago

Related: #371, #424

tsalo commented 4 years ago

I just want to note that I'm happy to close #424 in favor of a new PR or adapt it based on BEP-001 input, depending on what the BEP-001 team prefers.

KirstieJane commented 4 years ago

Ah - so this is a great question. I would say that I've suggested this a few times during our development of BEP001 and the major challenge that we've experienced is that the logic is really difficult to review separately 😭

@agahkarakuzu @Gilles86 - what do you think about trying to split aspects of the PR out?

Gilles86 commented 4 years ago

I don't see the need (it mostly seems like more work to me), but if a large group of reviewers find this helpful, of course we should accommodate.

So we would split up the PR into what? a) The new key/value-entities b) the new grouping suffixes and c) all the other stuff (e.g., explicit suggestions for MPM/MP2RAGE, etc?

agahkarakuzu commented 4 years ago

@KirstieJane it is not really easy to talk about qMRI data organization proposed in BEP001 without considering every component as a whole.

Even if the PR is somehow decomposed for the sake of easy communication, multiple PRs should exist simultaneously as part-1, part-2... and refer to each other.

Gilles86 commented 4 years ago

I agree with that point of @agahkarakuzu btw: I think all these three (?) subparts are connected. If you don't include multimodal anatomical data, there is not much use for these key/value-entities. The only reason I can think of right now that you would want grouping suffixes is if you have more complex anatomical data, etc...

KirstieJane commented 4 years ago

Yeah - just to clarify my comment above - I've suggested it and then we've had these same conversations about how hard they are to separate and then stopped!

I do think we could potentially update the main PR with a checkbox set of specific aspects that have changed though. That might help guide people as to specifically which aspects they would like to discuss...

effigies commented 4 years ago

How does the following sound?

If a reviewer feels that a piece is self-contained enough to discuss outside the full BEP, they split out that piece into a new PR (off of master). The initial PR should be identical to what is currently in BEP001, and any existing comments on the relevant sections should be copied and linked. Assuming the BEP team agrees that the section can be addressed separately, then they (or a maintainer) merge that branch into the #508 branch (should have no changes). Changes can be discussed to everyone's satisfaction, and merged into master. master can then be merged back into #508, which should go smoothly because of the earlier merge.

To take out the git wrangling (which I'm happy to do to allow everybody else to focus on the content), the process would be:

If a reviewer feels that a piece is self-contained enough to discuss outside the full BEP, they split out that piece into a new PR. The initial PR should be identical to what is currently in BEP001, and any existing comments on the relevant sections should be copied and linked. Assuming the BEP team agrees that the section can be addressed separately, the PR proceeds and changes can be discussed to everyone's satisfaction.

KirstieJane commented 4 years ago

That sounds amazing @effigies! :+1: from me. I think maybe we're too close to the BEP to see the places that can be pulled out easily!

Gilles86 commented 4 years ago

I'm really sorry for my ignorance, but I don't see how this works exactly. If the reviewer makes a branch that is identical to bids-bep001:master and then PRs that branch into bids-bep001:master, there will be no changes and not Github tools to discuss them, right?

And if they PR into bids-specification:master, all the commits of BEP001 will be in there right? How do we constrain it to the ones that are to be discussed?

KirstieJane commented 4 years ago

I think this is the git master-y that @effigies was willing to help with... I guess if it were me I'd probably just move some of the text into a new commit and start that but he may have clever-er ways to do that than me 😅

tsalo commented 4 years ago

I don't know if I'll be up to splitting them off, but here are some things that I think could/should be split off:

tsalo commented 4 years ago

Per yesterday's BEP001 call, the BEP team will decompose the main PR into the following:

  1. The flip entity
  2. The inv entity
  3. The mt entity
  4. The part entity (opened in #424).
  5. The suffix entity
  6. The qMRI-related changes.
    • This PR will also use all of the new entities proposed in the other PRs, so it is dependent on all of them, but can be reviewed semi-independently.

EDIT: Also, the concept of "deprecation" is introduced in a new, separate PR (#634), which reduces the load for BEP001.

sappelhoff commented 3 years ago

This process has been successfully completed. Please reopen if you need to still discuss points from here.