bids-standard / bep001

Project management repository (primarily issues) for BIDS Extension Proposal 001
Creative Commons Attribution 4.0 International
8 stars 11 forks source link

Created 'derivatives' folder with processed MPM output #38

Open lazaral opened 5 years ago

agahkarakuzu commented 5 years ago

@lazaral thanks! Small correction:

MTmap --> MTsat (magnetization transfer saturation index map)

Another concern regarding field maps, should we put them in the fmap folder @KirstieJane ?

lazaral commented 5 years ago

Good point, thank you @agahkarakuzu! The other maps are R1map R2starmap etc. - shall I go for "MTsatmap"?

I was also in doubt about the field maps as they're not "raw" data but they're also not the main output...

Gilles86 commented 5 years ago

@lazaral, great work!

Could you restructure the ds-04 a bit more, making a very clear distinction between derivatives and sourcedata? To be compeltely BIDSy, it should be something like

agahkarakuzu commented 5 years ago

@lazaral IMO we can keep this one as MTsat and let the R2starmap be the exceptionally long one, unless other people feel strongly otherwise :)

lazaral commented 5 years ago

@Gilles86 thank you, yes happy to restructure the folders! Btw what do you think of the other examples? I noticed that there the maps are also just in an 'anat' folder - should I move them to 'derivatives' and restructure the other 3 example datasets too?

Gilles86 commented 5 years ago

Yes! Would be great if you could do that. Otherwise I can also give it a shot.

lazaral commented 5 years ago

Great, restructuring all the examples now :)

lazaral commented 5 years ago

@lazaral thanks! Small correction:

MTmap --> MTsat (magnetization transfer saturation index map)

Another concern regarding field maps, should we put them in the fmap folder @KirstieJane ?

@agahkarakuzu I think the point made by @Gilles86 on derivatives being a higher-level folder might solves this problem, as we can put the field maps in derivatives/sub-01/fmap/ ... curious to hear if @KirstieJane also agrees?

lazaral commented 5 years ago

@Gilles86 Done! It's now added to the pull request :)

KirstieJane commented 5 years ago

@lazaral, great work!

Could you restructure the ds-04 a bit more, making a very clear distinction between derivatives and sourcedata? To be compeltely BIDSy, it should be something like

* `ds-04/derivatives/sub-01/anat/*_*map.nii.gz` for derivatives
  and

* `ds-04/sourcedata/sub-01/anat/*_*w.nii.gz` for sourcedata

Hey folks - I don't think this is quite right. sourcedata is for the stuff that comes off the scanner. The "bidsified" raw data (as in the raw data in BIDS format) should be in the same level as the derivatives directory: https://bids-specification.readthedocs.io/en/stable/02-common-principles.html#source-vs-raw-vs-derived-data.

So while the derivatives data in this PR are correct, I think the raw data should have the /sourcedata folder removed from the path.

eg:

I'll ping a PR to @lazaral's branch to show what I mean 😃

KirstieJane commented 5 years ago

Another concern regarding field maps, should we put them in the fmap folder @KirstieJane ?

@agahkarakuzu I think the point made by @Gilles86 on derivatives being a higher-level folder might solves this problem, as we can put the field maps in derivatives/sub-01/fmap/ ... curious to hear if @KirstieJane also agrees?

Heya - the field maps should go in the fmap folder I think. It is in the top level folder (neither sourcedata nor derivatives as above). Here's the current specification: https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#fieldmap-data

I think we need to update that section with an anatomical example. This conversation is already happening at #40.

[EDIT] Just as a quick question because I might have missed something. Why would the field maps go in the derivatives folder? Are they derived, as in, are they calculated off the scanner?

lazaral commented 5 years ago

@KirstieJane all makes sense - about the fields maps, my understanding is that with MPM both the B0 and B1+ field are calculated off the scanner in hMRI but others might be able to confirm.

lazaral commented 5 years ago

I've implemented some of the changes we've discussed on Wednesday - if people would like to have a look, any feedback is appreciated :)

(p.s. note that the FLASH/MEGRE and MPM suffices are still inconsistent, but have been fixed in other pull requests... not sure what the best thing to do is, but I have made a note of the inconsistencies in the current pull request so I'm happy to fix them manually here if helpful)

Gilles86 commented 5 years ago

So I heavily refactored the examples data in this branch https://github.com/Gilles86/bep001/tree/examples

Somehow, I cannot push it to lazaral/master:

> git push -u lazaral master
To github.com:lazaral/bep001.git
 ! [rejected]        master -> master (non-fast-forward)
error: failed to push some refs to 'git@github.com:lazaral/bep001.git'
hint: Updates were rejected because a pushed branch tip is behind its remote
hint: counterpart. Check out this branch and integrate the remote changes
hint: (e.g. 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Which is weird because when I do

> fetch lazaral master
> git merge lazaral/master

It says

Already up to date.

Anyone has a clue what's wrong here? @KirstieJane ?

Gilles86 commented 5 years ago

Then, @lazaral, could you have a look again at the MPM-examples?

Some issues to look into:

lazaral commented 5 years ago

@Gilles86 thank you for the work and suggestions - I think I need some further clarifications :)

lazaral commented 5 years ago

Quick summary of recent updates to this pull request: I've fixed a few small issue about file names that were raised at previous meetings, and added realistic jsons for all the niftis (anat and fmap files alike).

lazaral commented 5 years ago

One open question (@Gilles86 you might know?): previous versions of the MP2RAGE examples used to have a T1w scan, but I can't see it in the real example in OSF. Is that an error - should I remove it?

lazaral commented 5 years ago

Also, @agahkarakuzu MEGRE is not my specialty so the json files for that example are quite sparse sorry :) it might be nice to add some realistic details (e.g. repetition time and flip angle) if you have them at hand?