Closed rcwelsh closed 8 years ago
A few concerns
1) there is a non-trivial number of commits that are in merge head but not the base. This looks like it somehow merges a bunch of unrelated work (mostly/only the graph theory toolbox)
2) It looks like 3e01e3b introduces spm8 r6313, but this is also what #376 does. This update is essentially dependent on having spm8 r6313 introduced, and b/c of that dependency, it would make sense to have merged
Rather than accomplishing essentially the same delta in two different commits, it'd make sense to merge rcwelsh:core_beta/adding_spm8_with_R6313 into this branch directly. This would also simplify matters as any adjustments to #376 made during the pull request approval process can be merged back in here.
hi daniel,
correct, i first pushed 376, and then i branched locally on my machine from 376 too create 3e01e3b so that R6313 would be there for for the new spm8Batch. When I pushed 3e01e3b it said it needed the “upstream” flag. my interpretation was that if #376 is accepted then this one will accept easily.
I had branched both from universe/core_beta, so I’m not sure why all of graph theory stuff.
should i still merge?
-robert
On Nov 2, 2015, at 2:23 PM, Daniel A Kessler notifications@github.com wrote:
A few concerns
1) there is a non-trivial number of commits that are in merge head but not the base. This looks like it somehow merges a bunch of unrelated work (mostly/only the graph theory toolbox)
2) It looks like 3e01e3b https://github.com/UMPsychMethodsCore/MethodsCore/commit/3e01e3b3a026cccbad1d6bd061f61909dc904c72 introduces spm8 r6313, but this is also what #376 https://github.com/UMPsychMethodsCore/MethodsCore/pull/376 does. This update is essentially dependent on having spm8 r6313 introduced, and b/c of that dependency, it would make sense to have merged
Rather than accomplishing essentially the same delta in two different commits, it'd make sense to merge rcwelsh:core_beta/adding_spm8_with_R6313 into this branch directly. This would also simplify matters as any adjustments to #376 https://github.com/UMPsychMethodsCore/MethodsCore/pull/376 made during the pull request approval process can be merged back in here.
— Reply to this email directly or view it on GitHub https://github.com/UMPsychMethodsCore/MethodsCore/pull/377#issuecomment-153131283.
Ok, my bad! Didn't realize that 3e01e3b is common to both branches. Good on you! :+1:
However...
the spm8Batch updates should have started branched off of spm8Batch_beta, not core_beta. Because core_beta includes some other stuff (like graph theory), you are effectively back-porting everything that has happening on core_beta (including the spm8 updates you want, but also lots of stuff you don't want, like graph theory)
Here's my prescriptive flow that starts after you've already put together the spm8 R6313 updates (doesn't matter if pull request has been accepted or not)
1) branch off of spm8Batch_beta -> spm8Batch/update_for_R6313 2) merge core/add_spm8_R6313 into spm8Batch/update_for_R6313 3) pull request for spm8Batch/update_for_R6313
gotcha,
i’ll reconfig later this afternoon after i finish some ALS database matlab programming.
On Nov 2, 2015, at 2:39 PM, Daniel A Kessler notifications@github.com wrote:
Ok, my bad! Didn't realize that 3e01e3b https://github.com/UMPsychMethodsCore/MethodsCore/commit/3e01e3b3a026cccbad1d6bd061f61909dc904c72 is common to both branches. Good on you!
However...
the spm8Batch updates should have started branched off of spm8Batch_beta, not core_beta. Because core_beta includes some other stuff (like graph theory), you are effectively back-porting everything that has happening on core_beta (including the spm8 updates you want, but also lots of stuff you don't want, like graph theory)
Here's my prescriptive flow that starts after you've already put together the spm8 R6313 updates (doesn't matter if pull request has been accepted or not)
1) branch off of spm8Batch_beta -> spm8Batch/update_for_R6313 2) merge core/add_spm8_R6313 into spm8Batch/update_for_R6313 3) pull request for spm8Batch/update_for_R6313
— Reply to this email directly or view it on GitHub https://github.com/UMPsychMethodsCore/MethodsCore/pull/377#issuecomment-153135364.
After discussion with @rcwelsh via hangouts, it sounds like this pull request will be retracted in favor of one with a slightly different topology. However, I'd recommend you that reference this request in the comments on the new pull request so that there's a link to this discussion.
closing this pull request without the merge to make it clean.
Up updated spm8Batch to point to SPM8 revision 6313 will is in pull request #376. I tested the suite of spm8Batch commands from sliceTime8 all the way through warpfMRI (Using both standard warp and new segment). I also ran statistical analysis using both GUI and FirstLevel script.
@UMPsychMethodsCore/developers I'll need some +1's to get this through to public.
Only accept this pull request after #376