Major revisions to aaq_qsub (already merged with main branch) - #119
Merged
Conversation
Contributor
Author
|
Hopefully when you pull this changes git will update your local file
permissions. I've not tested that but I could quite easily.
…On 20 Jul 2017 4:00 p.m., "Tibor Auer" ***@***.***> wrote:
I am a fan of any consitency! :) Do I need to change anything in my repo
for that?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHtKBiJP4-cIqzHY_gmTmcyha2dHdV8Hks5sP2t3gaJpZM4OeK8z>
.
|
Contributor
Author
|
If you have git set to ignore permissions then nothing will change, and
your permissions won't affect the main repository.
…On 20 Jul 2017 4:00 p.m., "Tibor Auer" ***@***.***> wrote:
I am a fan of any consitency! :) Do I need to change anything in my repo
for that?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHtKBiJP4-cIqzHY_gmTmcyha2dHdV8Hks5sP2t3gaJpZM4OeK8z>
.
|
…ng pool.Jobs.Tasks error fields as a precaution
Collaborator
|
Looks good. Darren, can you pull the current AA master and push, and I will start the tests again (just merged a bunch of new stuff from Tibor)? No need to make a new pull request btw - this one should update automatically. |
Contributor
Author
|
Okay will try to do that today.
…On 8 August 2017 at 10:44, Johan Carlin ***@***.***> wrote:
Looks good. Darren, can you pull the current AA master and push, and I
will start the tests again (just merged a bunch of new stuff from Tibor)?
No need to make a new pull request btw - this one should update
automatically.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHtKBsaaqwAIBMo1tG7DULIe7IVnQzNSks5sWC3zgaJpZM4OeK8z>
.
|
* DEBUG: incomplete meminfo update * UPDATE: local parallel (by slices) execution switched from parpool to independent job submission (more robust on certain clusters) * volumes2movie: replaced depreciated movie2avi with writeVideo * DEBUG: obtaining version info from Git * DEBUG: remove job-specific aap from engines * DEBUG: allow access to aap from previous execution * DEBUG: allow access to aap from previous execution * DEBUG:facemasking - incorrect space information (FSL-SPM) * UPDATE: code refractoring * UPDATE: standalone pragmas * DEBUG: standalone without extra tools * add DS_Store to gitignore * minor fixes to aas_checkreg, mri_findvol, and aamod_coreg_noss * DEBUG: unified roi_valid for diffusion * support for struct arrays * only spmdir on MATLABPATH for robustness, raise exception if shell commands fail * bug fix: handle relative path in aap.directory_conventions.T1template * robust to problems creating figures e.g. when running on cluster * flexible handling of T1template - relative or abs path * revert bad try/catch * improvements * new optional sessnames and tasknames inputs, handle firstlevel_model_1_config * experimental support for sub- prefix names in aa_export_to_BIDS * bugfix: absolute paths work again * split up aamod_firstlevel_model into 3 stages: config,convolve,estimate * new module for modeling sub-runs as separate runs * new module for generating a mean normalised T1 * documentation * diagnostic_ instead of diagnostics_ for diagnostic outputs * DEBUG/UPDATE: allow optional arguments for custom dicom converter script * UPDATE: refracter/unify special-series so that they can be combined more easily. Session name determines - "modality" (ASL, MTI, etc) - stream names * NEW FEATURE: MPM
Member
|
@dprice80, I think you should not mix other improvements (e.g. changes to roi xml - remove specific stream names (aamod*00001)) with this PR. |
* DEBUG/UPDATE: allow optional arguments for custom dicom converter script * UPDATE: refracter/unify special-series so that they can be combined more easily. Session name determines - "modality" (ASL, MTI, etc) - stream names * NEW FEATURE: MPM
Member
|
@dprice80, are you sure it is safe to remove done flag check? |
Contributor
Author
|
Yes, this was just an additional check I inserted into aaq_qsub.m, To give
you the background: something changed with our Torque server, and errors
were not being reported correctly in the pool object (Russell doesn't know
why). To protect against this event, first I inserted an additional check
when the job goes to finished state to check whether an error message
exists in the Tasks object (which is still there). However, there were a
couple of occasions when the job quit without an error message, but was
reported as finished in the pool object. The job had not actually completed
so the next dependent module would never run. Therefore, I put an
additional check in aaq_qsub.job_monitor() to check that the done flag
exists. However, our file system does not update quickly enough for this to
work reliably (no idea why that is - perhaps this would work fine on other
systems). I still don't know what the root cause of the problem was, but it
is very rare, so I will wait and see if it happens again.
…On 24 August 2017 at 07:28, Tibor Auer ***@***.***> wrote:
@dprice80 <https://github.com/dprice80>, are you sure it is safe to
remove done flag check?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHtKBs7H016sBouxxxXRaYofzntkDmfkks5sbRgLgaJpZM4OeK8z>
.
|
Collaborator
|
Ok, so any further issues to discuss before I start running the tests? @tiborauer ? |
Member
|
Nothing on my side. |
Contributor
Author
|
I'm ready. I've committed the latest changes so nothing to add.
…On 24 Aug 2017 11:13, "Tibor Auer" ***@***.***> wrote:
Nothing on my side.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHtKBqVpdZkhSOywAyZ2eogJuAHiFTAnks5sbUzHgaJpZM4OeK8z>
.
|
Member
|
@dprice80, Again, I think you should not mix other improvements with this PR. |
Will add error code to a separate branch (to avoid problems with ongoing PR)
Contributor
Author
|
Okay, I have undone that last change, and created a new branch.
Darren
…On 4 September 2017 at 11:39, Tibor Auer ***@***.***> wrote:
@dprice80 <https://github.com/dprice80>, Again, I think you should not
mix other improvements with this PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#119 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHtKBsPryW9f86hMOzZaVcTXQsZoOyuBks5se9NggaJpZM4OeK8z>
.
|
Member
|
I am close to finish the test and will have a few commits to add. I will submit a PR to you, @dprice80. If you agree with them and accept them, this PR will be automatically updated, so that I can merge. |
Member
DEBUG/UPDATE PR119
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have merged with rhodri's main as requested in previous pull request