Improvements to first and second level thresholding - #159
Conversation
|
FYI: I just noticed bugfixes from my previous (rejected) PR are included in this PR for some reason -- these all get undone by the subsequent commits. |
|
I am sorry for not responding for long, but I have been thinking about how the solution for structural images. I think only aamod_norm_noss generates 2-element structural. We have recently discussed with Johan, that we could probably retire that module, because aamod_segment8 is the more current. However, even if we decide to keep it, we can easily modify the output structural. Then later we can deal with the 'consequences'. So I propose to have only one (non-essential and renameable) structural stream for aamod_firstlevel_thresholding (similarly to secondlevel). And SPMT1 can be used when no structural is found. |
We agree on the fix; we disagree on when (and who) gets to fix it. :-) My PR is about fixing Issue 150, and to do so without breaking any existing pipelines. It's not about fixing the multistream thing. That's why I added normalised_structural for now.
However, the following modules assume 'structural' might contain more than one stream: aamod_ANTS_struc2epi.m: Will deleting/changing norm_noss break pipelines that currently use these modules? Are they obsolete? Or do they need to be fixed? These are questions above my pay grade. |
|
I am saying that we should not fix an issue with knowing that we have to do it again soon. You should assume that structural contains only one image. We can quickly solve the structural issue by simply editing norm_noss. Whether or not it breaks pipelines, is a question for the next day; because we all agreed on that this is the right thing. We should only make sure that the example pipelines works. Tomorrow we can discuss it, and decide who, when and how will do it. |
|
In the past Rhodri has been a strong voice for not breaking existing
pipelines. I tend to be more flexible in that people who are downloading
new versions of aa are *probably* developers or brand new users (who don’t
have pipelines to break).
…On Tue, Aug 7, 2018 at 5:16 PM Tibor Auer ***@***.***> wrote:
I am saying that we should not fix an issue with knowing that we have to
do it again soon. You should assume that structural contains only one image.
We can quickly solve the structural issue by simply editing norm_noss.
Whether or not it breaks pipelines, is a question for the next day; because
we all agreed on that this is the right thing. We should only make sure
that the example pipelines works.
Tomorrow we can discuss it, and decide who, when and how will do it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#159 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGtPS0iZeIAafM_4FBm0nMcqXvpdUxtks5uOhHCgaJpZM4Vsozp>
.
|
|
I will remove normalised_structural from the header and assume 'structural' (if it is present) contains a single stream. Might take a few days to complete (have other deadlines this week)... |
|
I've added to the PR -- the thresholding modules now assume structural contains only one stream. |
|
I saw the commit. Thanks! I have already Started reviewing Johan's PR and will be ready this week, I hope. |
|
@jones-michael-s, I have noticed a few files which belongs to bugfixes:
|
|
@jones-michael-s, Another comment: If you include a function, likely used only by a single code/module, then you should rather add that function as a subfunction to the module to avoid extra files (in the repo and the path). Of course, an external function should go into the external folder with a corresponding entry in the README.json. |
This are files from my previous PR. You found a better fix for aamod_realign after I submitted the PR. Unfortunately git doesn't permit removing individual files from a PR so I deleted the whole thing. Unfortunately, I didn't create a new branch in my local repo for the changes, so the only way to undo the changes was to edit all the files back to the way they were by hand. Git is too stupid to understand the files are no longer changed. Instead, it insists on including the edits from the original PR in the current PR and then adding the edits that undid the previous edits.
I don't understand this comment. |
|
Why do not you take the original files from the 'central' stream and put them into this branch as a 'manual re-merge'? |
I believe that's what I did. I just did so using vi. The more pressing question is: What, specifically, is the problem with my current PR? |
|
I am only worried that it changes files other than intended by this PR. However, if you are happy with these changes:
then I can go ahead. |
|
I realize it looks odd, but I'm pretty sure the files are correct. However, could you hold off for just a bit -- I'm going to move spm_list_display_noUI.m into make_stats_table.m as a subfunction, and also update readme.json, like you suggested. I should be able to push these changes today. |
|
Sure! Thank you! |
Alas no. I've been neck deep in deadlines and cluster problems this month. |
|
Okay, I pushed my changes to spm_list_display_noUI.m and make_stats_table.m and README.json. |
|
Hi @jooh, Do you know, what is the reason of the conflict? Should @jones-michael-s first merge the recently accepted PR in his repo before we start reviewing this PR? |
|
Yes. We can't merge when there are conflicts. So the correct thing to do is git fetch origin, then git merge master (assuming origin is rhodricusack). You will have merge conflicts. Then go through all conflicts and resolve them, commit and push to your remote. The pull request should update automatically and the conflict above will go away. If you know that you want to resolve all conflicts in favour of rhodricusack rather than your own version, you can use the 'theirs' flag when merging to get git to automatically do this. See this thread. |
|
Hello all, I did what Johan suggested. I think it worked as advertised. |
|
Yes! Now, there is no conflict. 👍 |
|
I think I have accidentally managed to push my commit into @jones-michael-s's branch, which also updated his PR. |
Version upgrade
4 participants
I've added an explicit "template" parameter to both modules to help avoid incorrect mapping (cf. Issue aamod_firstlevel_threshold generating incorrect maps #150). The options are structural, normalised_structural, and SPMT1 template (firstlevel) and averaged_structural or SPMT1 (secondlevel). The default settings are normalised_structural and SPMT1, respectively, which should reproduce the current behavior so (fingers crossed) it shouldn't break any existing pipelines. Firstlevel_threshold even continues to check for a 2-element structural stream and will use the second (presumably normed) structural if present (we've talked about scouring this particular stream weirdness from the code, but I started looking into it and its a much bigger project than just aamod_firstlevel_threshold.) If the module doesn't find the normed structural, it will fall back to using the SPM T1 template, which was the old behavior as well.
Both firstlevel and secondlevel threshold now save all maps and renders (i.e., including those in which no voxels survive thresholding). We use a custom postprocessing tool at WashU that gathers all the analysis jpgs into an html montage for rapid review, and having random missing images makes the montage confusing. However, I realize saving zero-voxel images can be problematic (because you don't know if they are really zero-voxel images, or the map/render just missed a few). Ergo, I indicate zero voxel images by indicating "no voxels survive threshold" on the image.
I've added a "description" parameter. This is text is written to the upper right corner of jpegs created by the module. The text can be anything you like (I find it especially useful in branched analyses). Our design philosophy should be: analysis results should be self-documenting whenever possible. (Items 2 and 3 require a jpeg utility insertInImage.m which I downloaded from a Mathworks site and put in /external.)
I now generate SPM stats tables in firstlevel and add them to the report. This code isn't perfect -- it can only handle 'none' and 'FWE' thresholding, but it saves having to go back into SPM to get quantitative stats for these options. This functionality requires two auxiliary utilities: spm_list_display_noUI.m and make_stats_table.m, which I added to /extrafunctions.
I check in firstlevel_threshold if there is a possible problem with the template space. In secondlevel_threshold, I warn the user if they try to use FWE thresholding with a small number of subjects. (This may be an obvious mistake to experienced users, but I recall once getting stuck for a couple of days trying to figure out why my small-n second level analysis was crashing.) I've started adding these kinds of sanity checks to every module I modify -- any checking of module settings I can think of that might help the user not do something stupid (for example, map normed epis onto native structurals). A recurring theme in aa development is making the package more accessible -- especially to new users -- and I believe we should be adding checks everywhere we can. The code doesn't have to take extreme measures or try to correct the parameters -- just displaying an informative message is sufficient. This is not a new idea -- a handful of extant modules warn the user of of suspicious behavior (e.g., "multiple T1 found") but aa developers should really be much more diligent about this.