ENH: Drop missing model inputs #183

adelavega · 2019-09-24T21:46:04Z

Fixes #176

Behavior is toggled by CLI flag (--drop-missing). By default, it crashes.
In FirstLevelModel, empty columns are detected in design matrix.
In higher level models, dropping missing inputs is handled by MergeAll.

adelavega · 2019-09-24T21:47:33Z

Also, I'm wondering how to handle Nones with traits, as they will fail at the OutputSpec right now (and will also fail the Input spec on the next model).

codecov-io · 2019-09-25T17:46:54Z

Codecov Report

Merging #183 into master will decrease coverage by 0.3%.
The diff coverage is 67.56%.

@@            Coverage Diff            @@
##           master    #183      +/-   ##
=========================================
- Coverage    76.7%   76.4%   -0.31%     
=========================================
  Files          18      18              
  Lines         996    1017      +21     
  Branches      171     177       +6     
=========================================
+ Hits          764     777      +13     
- Misses        146     150       +4     
- Partials       86      90       +4

Flag	Coverage Δ
#ds003	`76.4% <67.56%> (-0.31%)`	⬇️

Impacted Files	Coverage Δ
fitlins/workflows/base.py	`60.43% <ø> (ø)`	⬆️
fitlins/interfaces/bids.py	`74.1% <ø> (+0.52%)`	⬆️
fitlins/cli/run.py	`80% <100%> (+0.18%)`	⬆️
fitlins/viz/reports.py	`89.55% <100%> (+0.48%)`	⬆️
fitlins/interfaces/abstract.py	`100% <100%> (ø)`	⬆️
fitlins/interfaces/nistats.py	`81.75% <50%> (-5.45%)`	⬇️
fitlins/interfaces/utils.py	`82.53% <78.57%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a41735...b78e34d. Read the comment docs.

effigies · 2019-09-25T17:56:59Z

fitlins/interfaces/nistats.py

+                        if v != 0 and n not in all_regressors]
+                       for row in contrast['weights']])
+        if missing:
+            weights = None


If we continue, then the contrast list is just shorter, and you don't need to deal with Nones.

Suggested change

weights = None

continue

Would this work? We'll need to make sure that there isn't some other place where we're separately generating a list that should match 1-1, but no longer would.

Was this not a sufficient place to block the production contrasts involving missing regressors?

adelavega · 2019-09-25T21:01:22Z

I have to test more, but this seems to have worked for me.

The main related issue is that if only one image per run remains, a second level moel can't be run, and it will crash.

This is the related issue: neuroscout/neuroscout#641
Seemed like it got more complex, but maybe in the meantime we can have the 2nd level model output the input if it only received one (instead of trying to run a model).

adelavega · 2019-09-25T21:02:28Z

If I'm deciphering correctly, as far as fitlins is concerned, we need to add pass through for random effects models, and implemented fixed effect models as well?

The rest of the issue is more about the spec and how to specific random vs fixed effects.

effigies · 2019-09-25T21:10:00Z

Yes... I need to re-read that, but it's also time to make dinner.

adelavega · 2019-09-25T21:16:33Z

Sounds more appealing, tbh.

adelavega · 2019-09-25T22:01:15Z

Allright, cool, this is ready for review. We can handle the fixed/random stuff in another PR, I think. That may end up being more complicated. Well at least the spec stuff, we could probably bang out the pass-through fairly quickly (wonder if that should be the default option + a warning, and if it needs a CLI flag).

effigies

Some very preliminary comments. I'll get back to this this afternoon.

effigies · 2019-09-26T13:11:23Z

fitlins/interfaces/utils.py

    output_spec = DynamicTraitedSpec

-    def __init__(self, fields=None):
-        super(MergeAll, self).__init__()
+    def __init__(self, fields=None, **kwargs):


drop_missing is relevant to the purpose of this interface in the overall workflow, but isn't very obvious in terms of functionality when considered on its own. What if we invert it to a check_lengths kwarg and just save it as an instance variable?

def __init__(self, fields=None, check_lengths=True): super(MergeAll, self).__init__() self._check_lengths = check_lengths ...

Then you can drop all of the input spec changes.

effigies · 2019-09-26T15:48:49Z

fitlins/interfaces/nistats.py

+                        if v != 0 and n not in all_regressors]
+                       for row in contrast['weights']])
+        if missing:
+            weights = None


Was this not a sufficient place to block the production contrasts involving missing regressors?

adelavega · 2019-09-26T17:04:40Z

I reverted the commit about pass through since it seems we need to implement FEMA for that.

…own function

adelavega · 2019-09-30T16:29:26Z

@effigies if the tests pass, and you approve, this should be ready.

i'll tackle adding FEMA contrasts in another PR (that might stay open for a while, since it will be blocked by nistats + spec changes)

adelavega · 2019-09-30T18:41:38Z

The main conflict is where to put the drop_missing input spec for what I think is now called DesignMatrix? I'm guessing this change should go into DesignMatrixInterface?

…enh/partial_runs

effigies · 2019-09-30T19:19:13Z

I think it needs to go in both DesignMatrix and FirstLevelModel, right? I'll review this now.

effigies

Hmm. Looks like you're not using drop_missing in FLM, so I guess it all just falls out of the design matrix?

fitlins/cli/run.py

fitlins/data/full_report.tpl

effigies · 2019-09-30T19:20:52Z

fitlins/interfaces/nistats.py

@@ -37,6 +43,9 @@ def _run_interface(self, runtime):
        import nibabel as nb
        from nistats import design_matrix as dm
        info = self.inputs.session_info
+        drop_missing = self.inputs.drop_missing


bool(Undefined) == False.

Suggested change

drop_missing = self.inputs.drop_missing

drop_missing = bool(self.inputs.drop_missing)

effigies · 2019-09-30T19:21:06Z

fitlins/interfaces/nistats.py

@@ -37,6 +43,9 @@ def _run_interface(self, runtime):
        import nibabel as nb
        from nistats import design_matrix as dm
        info = self.inputs.session_info
+        drop_missing = self.inputs.drop_missing
+        if not isdefined(drop_missing):
+            drop_missing = False


Then remove these lines.

fitlins/interfaces/nistats.py

effigies · 2019-09-30T19:22:33Z

fitlins/interfaces/nistats.py

            contrast_metadata.append(
                {'contrast': name,
                 'stat': contrast_type,
                 **out_ents}
                )
+            maps = flm.compute_contrast(
+                weights, contrast_type, output_type='all')


I think it just made sense to me to do that right before looping over it. No functional difference.

fitlins/interfaces/utils.py

fitlins/workflows/base.py

adelavega · 2019-09-30T19:26:19Z

I think only in DesignMatrix as that's where the errors are thrown. For higher levels they are thrown in MergeAll. I'm in the process of running a NS model with these new changes.

adelavega · 2019-09-30T19:29:34Z

I'm getting an error I don't understand:

Exception occurred in traits notification handler for object:
bold_file = <undefined>
contrast_info = <undefined>
design_matrix = <undefined>
mask_file = <undefined>
smoothing_fwhm = <undefined>
, trait: trait_added, old value: <undefined>, new value: drop_missing
Traceback (most recent call last):
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/traits/trait_notifiers.py",
line 591, in _dispatch_change_event
    self.dispatch(handler, *args)
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/traits/trait_notifiers.py",
line 553, in dispatch
    handler(*args)
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/nipype/pipeline/engine/nodes
.py", line 1045, in _set_mapnode_input
    setattr(self._interface.inputs, name, newvalue)
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/traits/trait_notifiers.py",
line 400, in __call__
    handle_exception(object, trait_name, old, new)
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/traits/trait_notifiers.py",
line 180, in _handle_exception
    raise excp
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/traits/trait_notifiers.py",
line 394, in __call__
    self.handler(*args)
  File "/opt/miniconda-latest/envs/neuro/lib/python3.6/site-packages/traits/has_traits.py", line
1168, in _trait_added_changed
    if (trait.type == "delegate") and (name[-6:] != "_items"):

effigies · 2019-09-30T19:30:11Z

Are you reusing a working directory?

adelavega · 2019-09-30T19:38:21Z

No, I'm not. This doesn't seem to happen on master fwiw.

adelavega · 2019-09-30T19:39:58Z

Ah, I see what it is, drop_missing was going to the first level model, whereas it should to design matrix now.

adelavega · 2019-09-30T19:46:08Z

I set drop_missing as a kwarg for DesignMatrix and took it out of the input_spec. Does that make sense to you?

effigies · 2019-09-30T19:50:32Z

I set drop_missing as a kwarg for DesignMatrix and took it out of the input_spec. Does that make sense to you?

From the nipype perspective, the contents of the outputs will not purely depend on the contents of the inputs, so you might get a situation where you re-run, adding or removing a --drop-missing flag, and the outputs from the previous run get reused, rather than recomputed. I think I would prefer it to be an input.

You can still keep the DesignMatrix(drop_missing=drop_missing), though. That will set the trait in the input spec.

adelavega · 2019-09-30T19:55:13Z

Okay, sure, then what I don't understand is why we don't do the same for MergeAll. Shouldn't that also be as an input then?

effigies · 2019-09-30T20:07:49Z

Yeah, strictly speaking, probably. It doesn't change the outputs, but instead changes the error condition, so it's a little less problematic. Also by the time things show up there, they will have had to go through DesignMatrix. So... whichever you want.

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

adelavega · 2019-10-01T02:43:44Z

Okay, your suggestions made sense, and I applied them. Regarding the input spec, I'll leave it as we have it. I'll test this out on NS tomorrow.

fitlins/interfaces/utils.py

effigies · 2019-10-01T17:22:26Z

fitlins/interfaces/nistats.py

+                variance_maps.append(_variances[0])
+                zscore_maps.append()
+                pvalue_maps.append()
+                stat_maps.append()


I think these three lines will cause problems?

Ah, yes, I thought I reverted that commit.

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies

Cool. Assuming tests pass, I'm good with this. This has been tested for your use case?

adelavega · 2019-10-01T21:29:27Z

Hmm, there seems to be a minor problem with the outputting of plots now, let me investigate

… variable logic

adelavega · 2019-10-02T04:21:32Z

Okay, tested and working. My linter suggested I change == to is and that screwed things up.

First pass at gracefully skipping missing inputs

2045322

Add CLI option for drop missing

1bda7d0

effigies reviewed Sep 25, 2019

View reviewed changes

adelavega added 2 commits September 25, 2019 14:10

Continue with missing value

2ff375a

Drop missing contrast_metadata

81edb96

Update reports to reflects missing contrasts

a79f83f

TEMP: Draft of pass through option

4606e4f

effigies reviewed Sep 26, 2019

View reviewed changes

adelavega force-pushed the enh/partial_runs branch from 4606e4f to a79f83f Compare September 26, 2019 17:03

adelavega added 2 commits September 26, 2019 12:57

Don't prepare contrast for missing

9a27f3c

Use kwargs for check_lengths in MergeAll, and refactor checking into …

0f63a39

…own function

adelavega changed the title ~~Skip missing model inputs~~ ENH: Drop missing model inputs Sep 30, 2019

Merge branch 'master' into enh/partial_runs

8219904

effigies mentioned this pull request Sep 30, 2019

RF: Abstract interfaces to simplify swappability #188

Merged

adelavega added 2 commits September 30, 2019 14:14

Merge master, fix conflicts with abstract interfaces

646a2a6

Merge branch 'enh/partial_runs' of github.com:adelavega/fitlins into …

48e59bc

…enh/partial_runs

effigies reviewed Sep 30, 2019

View reviewed changes

Drop missing as instance variable in DesignMatrix

3a70877

adelavega and others added 3 commits September 30, 2019 21:39

Minimal change to CLI args

e280bfd

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

Improve report aesthetics

af06ea4

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

Apply suggestions from code review

c277e33

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies reviewed Oct 1, 2019

View reviewed changes

fitlins/interfaces/utils.py Show resolved Hide resolved

effigies reviewed Oct 1, 2019

View reviewed changes

adelavega and others added 2 commits October 1, 2019 15:24

Reset self._lenghts

63d767c

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

Revert passthrough

b0b995f

effigies approved these changes Oct 1, 2019

View reviewed changes

Set drop_missing as as an input to DesignMatrix, and correct missing…

b78e34d

… variable logic

adelavega force-pushed the enh/partial_runs branch from ec9d42c to b78e34d Compare October 2, 2019 04:20

effigies merged commit f822f3c into poldracklab:master Oct 2, 2019

adelavega deleted the enh/partial_runs branch October 2, 2019 16:16

adelavega mentioned this pull request Aug 9, 2021

FIX: Take the intersection of variables and Model.X, ignoring missing variables (usually contrasts) bids-standard/pybids#764

Merged

	drop_missing = self.inputs.drop_missing
	drop_missing = bool(self.inputs.drop_missing)

ENH: Drop missing model inputs #183

ENH: Drop missing model inputs #183

Conversation

adelavega commented Sep 24, 2019 • edited

adelavega commented Sep 24, 2019 • edited

codecov-io commented Sep 25, 2019 • edited

Codecov Report

effigies Sep 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelavega commented Sep 25, 2019

adelavega commented Sep 25, 2019

effigies commented Sep 25, 2019

adelavega commented Sep 25, 2019

adelavega commented Sep 25, 2019 • edited

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelavega commented Sep 26, 2019

adelavega commented Sep 30, 2019

adelavega commented Sep 30, 2019

effigies commented Sep 30, 2019

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelavega commented Sep 30, 2019

adelavega commented Sep 30, 2019

effigies commented Sep 30, 2019

adelavega commented Sep 30, 2019

adelavega commented Sep 30, 2019

adelavega commented Sep 30, 2019

effigies commented Sep 30, 2019

adelavega commented Sep 30, 2019

effigies commented Sep 30, 2019

adelavega commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

adelavega commented Oct 1, 2019

adelavega commented Oct 2, 2019 • edited

adelavega commented Sep 24, 2019 •

edited

adelavega commented Sep 24, 2019 •

edited

codecov-io commented Sep 25, 2019 •

edited

effigies Sep 25, 2019 •

edited

adelavega commented Sep 25, 2019 •

edited

adelavega commented Oct 2, 2019 •

edited