bids-standard / bids-bep016

BEP016: diffusion derivatives
Creative Commons Attribution 4.0 International
6 stars 7 forks source link

Major refactoring for upstream changes / blocks #92

Closed Lestropie closed 1 month ago

Lestropie commented 2 months ago

Replaces #90.

Resolving the BEP against the absence of both complex inheritance and suffix distinction is not as simple as removing complex inheritance and swapping suffices; attempting to do so just breaks everything around it. But given prior precedent I'm pessimistic about my ability to convey this convincingly. So I'm better off making the changes necessary to conform to the main structural constraints (no IP changes, single fixed suffix) that will both retain compatibility with extension beyond the most trivial of diffusion models, and hopefully not be objectionable on other bases.

I've ended up making a much broader scope of changes than just "doing #90 differently", as there are too many moving parts to handle in isolation. I'll try to break up my description here in terms of aspects that address those core structural constraints, vs. other things that to greater or lesser necessity got changed along the way. I can't finish this off right now as I have other urgent tasks, but hopefully this is enough to elicit opinions.

Primary structure

Other changes

Outstanding questions

arokem commented 1 month ago

Any objections to merging this into #24 so that we can fix up things there and keep moving towards a PR on the bids-standard/bids-specification? If I get no objections by mid next week, I will go on ahead with that and we can follow up with more PRs on top of this one (specifically to address the "notes" and "todo" sections of current PR).

Lestropie commented 1 month ago

I'm eager to merge it to start chipping away at other things. I've been writing code to verify appropriate interpretation of fibre orientations (not at the scope of #23, mostly trying to get an answer to #96), and have already identified one aspect where the spec will need to be augmented to support FSL data.