VUIIS / dax

Distributed Automation for XNAT
MIT License
25 stars 24 forks source link

Handle multiple scans with same type; allow not building for unusable scans #353

Closed baxpr closed 2 years ago

baxpr commented 2 years ago

Fixes #352.

Adding two new options for yaml processors, for the section inputs.xnat.scans. Neither is required to be present.

keep_multis can be all (to obtain current behavior); or first, in which case when multiple scans with the same scan type are found as potential inputs to an assessor, only the first one is kept in the list. "First" is defined as first in alphabetical order by the scan ID string.

skip_unusable can be False (to obtain current behavior where assessors are built for unusable scans but marked Unusable in the QC field); or True, in which case assessors are just not not built for scans that are marked unusable.

These options are not implemented for inputs.xnat.petscans or inputs.xnat.assessors.

TESTED in a session with two same-named T1 scans:

Testing skip_unusable: With existing yaml options: OK - Regular build no extras, T1s both usable, expect two assessors at NEED_INPUTS OK - First T1 unusable and no needs_qc - expect two assessors at NEED_INPUTS OK - First T1 unusable WITH needs_qc - expect two assessors but one marked unusable With new skip_unusable field set in the yaml (but not needs_qc): OK - Build with first T1 unusable - expect only 1 assessor. OK - Build with both T1 usable. Expect 2 assessors at NEED_INPUTS

Testing keep_multis: OK - Set keep_multis 'all', T1s both usable, no needs_qc, no skip_unusable, expect two assrs at NEED_INPUTS OK - Same but keep_multis 'first', expect one assr for first T1

baxpr commented 2 years ago

Note to self: update docs

baxpr commented 2 years ago

One thing to think about - right now the "first" option works for identically named scans with identical type values. It could be quite handy if it operated over the regex instead, i.e. 'T1_1' and 'T1_2' are considered replicates if the requested type was 'T1*'

baxpr commented 2 years ago

Updated for "first" to operate over the regex. Additional testing for keep_multis:

OK - Set keep_multis 'all', T1s both usable, no needs_qc, no skip_unusable, expect two assrs at NEED_INPUTS OK - Same but keep_multis 'first', expect one assr for first T1 OK - T1 and T1_1 with T1 match regex, keep_multis 'all' OK - T1 and T1_1 with T1 match regex, keep_multis 'first'

baxpr commented 2 years ago

Really should test on an actual in-use multi-processor pipeline e.g. cat12-connprep-conncalc to see if the right stuff gets generated.

baxpr commented 2 years ago

Testing on a "real" scenario: cat12(t1) connprep(t1,fmri,cat12) conncalc(fmri,connprep)

Both T1s match the regex for all assessors One fmri is marked unusable connprep is to take the 'first' cat12 and match the t1 to cat12 conncalc is to match the fmri to connprep

There are 2 matching T1s and 4 matching fmris. With no constraints, we would have 2 cat12 4 fmri x 2 cat12 = 8 connprep 4 fmri x 8 connprep = 32 conncalc

By forcing the match to T1 we reduce that to 2 cat12 8 connprep 8 conncalc

We can prevent the redundant 4 connprep and 4 conncalc from actually running if we mark a T1 unusable and use needs_qc on the T1 in the conn* yamls. That is the best we get with the existing.

With the new restriction options, we should get 2 cat12 3 connprep (take 'first' T1, match T1 to cat12, skip unusable fmri) 3 conncalc (match connprep to cat12, match fmri to connprep, skip unusable fmri) And that is exactly what I got in the testing.

Some of the match filters in there may be redundant, but we want them for safety/clarity anyway

bud42 commented 2 years ago

Looks good to me. I'm wondering if we also need a "last" option. In my experience, we typically want to use the later scan when there's a repeat.

baxpr commented 2 years ago

@bud42 I was thinking about allowing an index 1,2,3 also. Maybe should go ahead and do that huh

baxpr commented 2 years ago

Although most typical scenario for wanting the later scan would be compatible with just marking the first one unusable I think?

bud42 commented 2 years ago

yep, works for me

On Wed, Mar 16, 2022 at 3:38 PM Baxter P. Rogers @.***> wrote:

Although most typical scenario for wanting the later scan would be compatible with just marking the first one unusable I think?

— Reply to this email directly, view it on GitHub https://github.com/VUIIS/dax/pull/353#issuecomment-1069542783, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWLZ4UH3RBRMKMBUD5MR5TVAI2CNANCNFSM5P6CNZEA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

baxpr commented 2 years ago

Updated accordingly. Tested first, last, 2 (legit), 3 (too high), and the previous full test with three assessors.

baxpr commented 2 years ago

@bud42 if you like this, can you release+install? I've got a couple pending requests that need it. Thanks!