bids-standard / bids-bep016

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

Current state of the BEP (NOT FOR MERGING) #24

Open Lestropie opened 3 years ago

Lestropie commented 3 years ago

This is a replacement of #6 as the BEP is being reinvested in.

The PR is not for merging, but to show the current state of the difference between the BEP and an up-to-date master of the BIDS specification.

codecov[bot] commented 2 years ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

arokem commented 2 years ago

Looks like there are still a lot of unrelated things even after merging #78 in here.

arokem commented 2 years ago

Ok - now merged with the specification master branch, this is (for now) the actual diff against the specification.

arokem commented 1 year ago

Updated with current master branch on the upstream fork of the spec.

francopestilli commented 6 months ago

@Lestropie @arokem after this is approved, can we please merge this into the main spec? @effigies

arokem commented 6 months ago

I believe that next steps are:

  1. Review and eventually merge #92 into this PR. I am in the midst of a review of that one, which I hope to complete today.
  2. Move this branch into a PR against bids-standard/bids-specification, so that we can hash out some of the remaining questions (see https://github.com/bids-standard/bids-bep016/pull/92#issue-2264977772) in that broader format.
  3. After iterating there and coming to consensus, we would still need to complete all the work on the BEP, before it can be merged into the spec. The next two steps are: (a) Examples and (b)Schema. The BEP cannot be considered ready to merge before these steps are completed (see this page for details).

Maybe you were referring specifically to item 2 above when you said "merge into the main spec"?

francopestilli commented 6 months ago

Ok. Thanks let me know if I can help with the review

Lestropie commented 6 months ago
francopestilli commented 6 months ago

Hi @Lestropie it is my understanding that the full issue list cannot be addressed as it goes beyond the scope of the PR as it requires changes to BIDS. I proposed constructing the PR and discussing what is needed given what BIDSbis now and discussing the other issues in the context of BIDS 2.0. we are 4 years in in some major issues without working solutions. I think it is fair to ask to wait for BIDS 2.0 for your major requests.

arokem commented 6 months ago

I just merged the upstream master branch into this one, so this is in sync with bids-standard/bids-specification

arokem commented 5 months ago

IIUC, we need to resolve this discussion: https://github.com/bids-standard/bids-bep016/pull/104/files#r1605423106, and after that we can convert this to a PR against bids-standard/bids-specification:master. Is that others' understanding as well? Do folks want to weigh in on that terminology discussion? @francopestilli : do you have any thoughts on nomenclature for l/lmax?

Lestropie commented 5 months ago

A PR without #71 and with no exemplar data generated via #23 would IMO be laughed off. You can't demand that the community format their data to your decree whilst literally never having done it yourself. Please give me time to generate a BIDS App to generate and save the derivatives currently exemplified in the specification document, so that they can be moved from there to bids-examples. I had to deal with the reference frame handling issues first.

PeerHerholz commented 5 months ago

Hi @Lestropie,

concerning this, I started working on a respective BIDS App a while ago: bids_bep16_conv. It's a fully working BIDS App and we would only need to update the output formatting to conform to the current up-to-date version. Thus, if you would consider this feasible instead of starting a new BIDS App, I'm happy to help! No worries of course if not, e.g. you would prefer a different implementation, etc. .

Cheers, Peer

Lestropie commented 5 months ago

My comment was on the premise that the converter was being restricted in scope to exclusively conversion of pre-generated model fitting outputs, in which case another tool would be required to run the fitting then the conversion. Been a long break between bursts so certain details will still be a bit fuzzy. If in its current state that tool does both fitting and conversion, then that's the obvious starting point; but as per https://github.com/PeerHerholz/bids_bep16_conv/issues/7 longer term we'll want to think about separation of those two steps.