Suggestion: (Partially) Decompose BEP-001

[effigies]

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 #508 (comment)

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]

Related: #371, #424

[tsalo]

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]

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]

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]

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

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]

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]

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]

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

[Gilles86]

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]

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]

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:

• The concept of "legacy" suffixes.
• Each of the new entities (part, fa, inv, and mt).
  • By evaluating these separately, reviewers can also help identify which non-anatomical datatypes each entity should also apply to. As it stands, they are only described with respect to BEP-001's focus- anatomical MRI data.
  • Re @Gilles86's comment- at minimum, part has uses beyond complex anatomical MRI data, and violates BEP-001's assumption of interchangeability, as I understand it.
• Symbolic linking of scanner-derived processed data between the dataset and derivatives.
  • This has implications beyond qMRI, as scanners can do all sorts of processing, and is not required for the BEP. Files could, for example, just be copied rather than symbolically linked.
• Updated definition of RepetitionTime with an eye toward anatomical scans.

[tsalo]

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 [ENH] Add part entity for complex-valued data #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]

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