New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

added script to create jsons files for MP2RAGE #101

Merged

emdupre merged 15 commits into bids-standard:master from Remi-Gau:MP2RAGE-jsons

Nov 2, 2018

Collaborator

Remi-Gau commented Oct 3, 2018

Just a little python script to create the appropriate JSON files for MP2RAGE sequences in the relevant folders in a BIDS data set (see #99 )
PS: first python script ever, be gentle plz. :-)


          added script to create jsons files for MP2RAGE

bbd5a92

Member

dorahermes commented Oct 12, 2018

Can someone with Python & MPRage experience have a look at this? Not sure whom, trying @emdupre @KirstieJane

emdupre reviewed

View reviewed changes

Collaborator

emdupre left a comment

This is great ! Thanks so much, @Remi-Gau

I had a few comments on ways you could improve the readability of the code, and a few questions on what was happening so that hopefully we could streamline a little more ! Looking forward to seeing this integrated 👍

pythonCode/create_MP2RAGE_json.py Outdated

+              DEFINE CONTENT OF JSON FILES
+              '''
+              # sub-*_inv-1_MPRAGE.json
+              data_inv_1 = OrderedDict()

Collaborator

emdupre Oct 12, 2018

One option that might be a little cleaner -- since you know all of the key-value pairs at the start, you could include all of them in the definition. So here, for data_inv_1, you might do:

data_inv_1 = OrderedDict([
      ('InversionTime', '900'),
      ('FlipAngle', '5')
])

Totally stylistic, but might be nice from a human readability perspective !

pythonCode/create_MP2RAGE_json.py Outdated

+              data_inv_1['FlipAngle'] = '5'
+              # sub-*_inv-2_MPRAGE.json
+              data_inv_2 = OrderedDict()

Collaborator

emdupre Oct 12, 2018

Same thing for this dictionary !

pythonCode/create_MP2RAGE_json.py Outdated

+              data_inv_2['InversionTime'] = '2750'  # ms
+              data_inv_2['FlipAngle'] = '3'
+              # sub-*_T1map.json

Collaborator

emdupre Oct 12, 2018

Could you remove these lines since they're unused and not clarifying comments ?

pythonCode/create_MP2RAGE_json.py Outdated



		# sub-*_MPRAGE.json
		data_MP2RAGE = OrderedDict()

Collaborator

emdupre Oct 12, 2018

Same stylistic comment is above -- you can really see the difference here with many fields !

pythonCode/create_MP2RAGE_json.py Outdated

+              from collections import OrderedDict
+              '''
+              DEFINE CONTENT OF JSON FILES

Collaborator

emdupre Oct 12, 2018

Since these are single line comments, can you just do them in the # format rather than as block quotes ?

pythonCode/create_MP2RAGE_json.py Outdated



		'''
		WRITE THEM

Collaborator

emdupre Oct 12, 2018

Same thing here about single-line comments !

pythonCode/create_MP2RAGE_json.py Outdated

+              start_dir = ""  # insert here path to your BIDS data set
+              # list all subjects
+              subj_ls = next(os.walk(start_dir))[1]

Collaborator

emdupre Oct 12, 2018

instead of using os.walk, have you thought about using glob ? It's another standard import and might be a little cleaner, here !

Collaborator Author

Remi-Gau Oct 18, 2018

Still gotta do that one.

pythonCode/create_MP2RAGE_json.py Outdated

+                              # sub-01_T1map.json
+                              # sub-01_T1w.json
+                              data_T1['BasedOn'] = \

Collaborator

emdupre Oct 12, 2018

This is a little hard to read -- could you define these strings first and then assign them as a value to this key ?

pythonCode/create_MP2RAGE_json.py Outdated

+                              with open(os.path.join(json_folder, json_name), 'w') as ff:
+                                  json.dump(data_MP2RAGE, ff, sort_keys=False, indent=4)
+                              # sub-01_T1map.json

Collaborator

emdupre Oct 12, 2018

I'm not sure if these are clarifying or not ! Could you add more text, if they're intended to be ?

pythonCode/create_MP2RAGE_json.py Outdated

+                              with open(os.path.join(json_folder, json_name), 'w') as ff:
+                                  json.dump(data_T1, ff, sort_keys=False, indent=4)
+                              json_name = name[:12] + '_T1w.json'

Collaborator

emdupre Oct 12, 2018

I can't test this locally right now, so just to confirm -- why is it name[:12] ?

Collaborator Author

Remi-Gau Oct 18, 2018

why is it name[:12]
Well spotted! That was one issue I had to fix. The script will now write the JSON files if it finds a _inv1_part-mag_MPRAGE.nii.gz and the name of the JSON files to create can then be based on the end index of that file without worrying how people decide to name their subjects / session...

Collaborator

emdupre Oct 18, 2018 •

edited

Loading

Ah, I see now ! One other way to do this, then, that might be a little clearer -- something like:

sid = '_'.join(name.split('_')[:-3])  # excludes last three key-value pairs

Avoiding counting characters just eases the cognitive burden when reading the code ! And writing it, too :)

Collaborator

emdupre Oct 18, 2018

Also, then, rather calling e.g. name[:-29] multiple times, you could just assign the above string to a value and pop that in as you need it ✨

Collaborator Author

Remi-Gau Oct 19, 2018

Thanks for the feedback: helps me learn new stuff! Will implement that and directory walking today hopefully.
Also the multiple calling is definitely a bad habit from years of matlab coding: gotta lose that one fast!

Remi-Gau added 4 commits

October 18, 2018 15:26


          remove unecessary block quotes

8fb02cf


          made OrderedDict declaration more human readable

eacb65e


          made comments more transparent

63a6da1


          made file naming relative to end index of the mag image of the first …

248976a

…inversion

Collaborator Author

Remi-Gau commented Oct 18, 2018

I have covered most the changes suggested expect the glob thing.
I did not have my dataset with to test the script on so I wrote a quick and dirty bash script to create a dummy data set with MP2RAGE data (maybe it could be repurposed for other things) https://gist.github.com/Remi-Gau/6d179e8b59ab9c985369cba2f7282e94

Collaborator

emdupre commented Oct 18, 2018

Thanks, @Remi-Gau ! Great to see this moving forward 🚀
I added one more suggestion about deriving the unique subject / session string, but this is overall in good shape. I do think we could clean up the directory walking a bit more, and I'm happy to help give pointers for doing so if you're up for it !

Remi-Gau added 4 commits

October 19, 2018 14:26


          remove hard coded values

6cf0137


          remove dead code

7481d73


          implement string slit to get basename of files to create

1e21cfd


          implement glob instead of walk to get subject folder list

258cd26

Collaborator Author

Remi-Gau commented Oct 19, 2018 •

edited

Loading

@emdupre I have tried implementing a few of your suggestions.
Implementing glob to iterate throught the subject folders was pretty straightforward but I am much less sure how to go about doing the same for the following section given that some data set might not have a session level (i.e how to beautifully cover the sub-*/anat and sub-*/ses-*/anat cases without a lot of if statements).

subj_dir = os.walk(iSubj)
# go through all the files for that subject
for path, subdirs, files in subj_dir:
    for name in files:```

emdupre added 2 commits

October 26, 2018 08:09


          Linting, update line lengths

b46c5f0


          Use py3 glob features

c2f7d5c

emdupre force-pushed the MP2RAGE-jsons branch from c2f7d5c to 258cd26 Compare

October 26, 2018 12:41

Collaborator Author

Remi-Gau commented Oct 27, 2018

Hum... Silly question from a git / github beginners.
@emdupre I got notified of your 2 new commits : here and there.

A bit of googling and some fascinating and thrilling reads of the git doc taught me a lot of things but did not answer my question.

e.g git ls-remote tells me that your commits don't show on this PR.
$ git ls-remote upstream
258cd26 refs/pull/101/head
6bacdaf refs/pull/101/merge

But I don't think that commits live somewhere in the ether unattached to anything so I am most unclear how I can get them on my machine to actually test them. I am sure I am missing somthing but not sure what.

Collaborator

emdupre commented Oct 27, 2018

Yes, sorry, I had accidentally committed directly to your pull request at first ! I had forgotten I had write permissions. To fix it, I opened a new PR with the desired commits and then rebased your branch to remove the extra commits, which was definitely confusing 😄

If you go to your fork, you should see that there's an open PR. On the PR, it will tell you where the commits are coming from in a fork:branch format.

So in this case, it says emdupre:fix-pr since it's my fork and the branch (fix-pr) I created when I realized I had committed directly to your PR !

You can git clone that code to test it directly. Let me know if that helps, or if I can clarify anything else !

Collaborator Author

Remi-Gau commented Oct 28, 2018

Ha. That makes sense. I was getting confused as to what was going on.
Will check your code and get back to you promptly!


          bring start_dir declaration at top of the script

f45dc86

Collaborator Author

Remi-Gau commented Oct 29, 2018

Everything checks out. So I rebased your PR on top of mine and pushed the whole thing here.

I just wanted to make a PR to show what I was talking about with using glob. I also did a little bit of linting to be more pythonic.

Yup that helped a lot and was way quicker than doing a lot of googling in all possible directions!
Also I should definitely lint more!

The big thing, here, is that the features I introduced are not python2 compliant. So, it's really up to you whether or not you're interested in using them. Personally, I no longer support python2 in my own work, so this is something I'm completely fine with, but it's completely your decision if you still want to support it in your own code. Just let me know what you think !

As much as I try to be conservative with matlab and SPM (preference for backward and octave compatibility) I am just getting started with python so I went in straight thinking that I would stick to python 3. But I am still at a stage where I don't know what would break backward compatibility. So having only python 3 is fine by me.

emdupre added 2 commits

October 31, 2018 17:35


          [ENH] Switch to functions for writing json files

3258e35


          Merge branch 'MP2RAGE-jsons' into enh/create-functions

d56d6b1

Collaborator

emdupre commented Nov 1, 2018

Great, glad it was helpful @Remi-Gau !

I did one other thing that I thought might be helpful to you and to the reader, which is to try and minimize duplicated code by putting it in a function. It's now a PR on your fork -- let me know what you think !

Collaborator Author

Remi-Gau commented Nov 2, 2018

Just saw that. Looks very good to me (and it helps me learn new python functions I did not know).


          Merge pull request #2 from emdupre/enh/create-functions

bbd0e0d

[ENH] create functions

Collaborator

emdupre commented Nov 2, 2018 •

edited

Loading

Great, glad it was helpful ! ✨

This is a +1 on merging from me, then. Thanks so much, @Remi-Gau

EDIT: Will merge in today unless I hear otherwise from @KirstieJane @dorahermes

emdupre merged commit 0e26df6 into bids-standard:master

emdupre mentioned this pull request

python script to generate MP2RAGE json #99

Closed

Remi-Gau deleted the MP2RAGE-jsons branch

December 28, 2020 12:19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet