ENH: Enable smoothing at any analysis level #135

adelavega · 2019-04-24T21:35:05Z

Closes #134

Main issue of debate is the CLI argument. We could split it into two arguments, but I think its fine how it is.

adelavega · 2019-04-25T00:10:59Z

Looks like the fail is unrelated:

ERROR: Error installing 'file:///src/fitlins': editable mode is not supported for pyproject.toml-style projects. pip is processing this project as pyproject.toml-style because it has a pyproject.toml file. Since the project has a setup.py and the pyproject.toml has no "build-backend" key for the "build_system" value, you may pass --no-use-pep517 to opt out of pyproject.toml-style processing. See PEP 517 for details on pyproject.toml-style projects.

adelavega · 2019-04-25T01:00:27Z

pypa/pip#6433

…/smoothing

effigies · 2019-04-29T15:18:14Z

fitlins/cli/run.py

@@ -96,10 +96,11 @@ def get_parser():
                        help="use BOLD files with the provided description label")

    g_prep = parser.add_argument_group('Options for preprocessing BOLD series')
-    g_prep.add_argument('-s', '--smoothing', action='store', metavar="TYPE:FWHM",
+    g_prep.add_argument('-s', '--smoothing', action='store', metavar="TYPE:FWHM:LEVEL",


I would prefer the levels to be named, e.g. run, session, etc. So instead of -s iso:5:1, it would be -s run:iso:5.

It doesn't seem likely that we'll want to smooth multiple times, does it?

effigies · 2019-04-29T15:24:31Z

Dockerfile

@@ -100,7 +100,7 @@ RUN bash -c "source activate neuro \
    && sync

 RUN bash -c "source activate neuro \
-    &&   pip install --no-cache-dir -e \
+    &&   pip install --no-use-pep517 --no-cache-dir -e \


I'm fixing this in #136.

effigies · 2019-04-29T19:35:38Z

Rebased the PEP517 fix into #137 and merged. Please merge/rebase the latest master.

Dockerfile

effigies · 2019-04-30T19:50:39Z

This requires resolution again. Sorry. Also, any objection to switching to LEVEL:TYPE:FWHM, where LEVEL is a string instead of an int?

adelavega · 2019-04-30T20:45:11Z

What kind of strings? How do you refer to the group level?

effigies · 2019-04-30T20:53:24Z

Step names. So run, session, subject, dataset. And group = dataset.

adelavega · 2019-04-30T20:56:02Z

Of course (duh). That works for me.

adelavega · 2019-04-30T22:21:50Z

Okay, cool. This is ready to go. I'm not able to test it right now because for some reason our workstation is down. Give it a go @effigies? Otherwise, I'll test tomorrow if our workstation/server is back online.

effigies

This looks good. A few changes, and I'll test in the morning.

fitlins/cli/run.py

fitlins/workflows/base.py

effigies · 2019-05-01T13:23:33Z

One last thing to consider is what if we have a two-level model with steps "run" and "dataset", but I might for computational reasons say "I want to smooth after the run level, whether that next level is session, subject or dataset", but I don't want to inspect the model and modify my command line for each specific model.

I see a few options:

Quietly fail to smooth if the smoothing level doesn't match some model step.
Warn/error if the smoothing level doesn't match some model step.
Interpret "session" as applying to the first level session or above (and similarly for other levels).
Permit l1, l2, etc. to explicitly indicate a positional step.

Do you have any preference? From a minimal parsing option, 3 is appealing, but I think 4 + 2 will be the least surprising.

Co-Authored-By: adelavega <aleph4@gmail.com>

adelavega · 2019-05-01T15:06:09Z

I had similar concerns when I wanted to add negative indexes (e.g. I want to smooth at the last step (-1))

I would say 2 + 4 is a sensible plan, although I think its a bit of a corner case. I suspect most ppl will smooth either at the first or last level.

tyarkoni · 2019-05-01T15:07:23Z

What are valid values for type?

effigies · 2019-05-01T15:08:17Z

What are valid values for type?

Right now, just iso for isotropic. SUSAN smoothing is planned for the future.

effigies · 2019-05-01T15:18:36Z

It's mostly the fact that they're so closely related that it seems weird to break them into different options.

Additionally, if there's any chance people will want to smooth multiple times at different levels, the current approach allows us to do something like --smooth l1:susan:2 dataset:iso:5 or something. (I'm not sure if that's ever something we'd want, but breaking them apart will make that much worse.)

tyarkoni · 2019-05-01T15:24:04Z

One other thing to keep in mind is that smarter algorithms may need additional parameters... e.g., SUSAN has several. I don't know if these need much twiddling in practice, or if the defaults work well pretty much everywhere, but there's something to be said for using a simple, dumb approach as the default, and letting people explicitly indicate if they want to use something more complex.

I have a hard time imagining a scenario where someone would want to smooth twice, so I don't know if that's even worth supporting right off the bat... it just gives people one more researcher degree of freedom to play with something that would be hard to justify on principled grounds.

adelavega · 2019-05-01T15:27:19Z

Realistically, how many smoothing algorithms will we really support anyways? Probably only SUSAN and Iso, right?

If so, we could just make them two separate arguments with their own sensible defaults.

Tal, would you think smoothing prior to the group level (as we are), or after the first level is most sensible as a default?

effigies · 2019-05-01T15:29:55Z

Sure, let's do FWHM:LEVEL:TYPE, then. I'd check for param[0] == 'iso', though to interpret it as <FWHM>:l1:iso and emit a FutureWarning that the options have changed. This parameter was added because people needed it, so there might be a few scripts that depend on that behavior that I'd rather not break.

effigies · 2019-05-01T15:32:10Z

My feeling is that the expectation is generally going to be to smooth before any fitting, but I'm okay with violating the expectation as long as it's a defensible default.

adelavega · 2019-05-01T15:32:45Z

I agree that's probably the expectation as well. So I'm fine with that being the default.

tyarkoni · 2019-05-01T15:42:11Z

Agree with all of the above.

tyarkoni · 2019-05-01T15:47:44Z

Actually, just to clarify: my expectation re: smoothing level would be that it happens as early as possible. Where that becomes ambiguous is when the type of input can vary. I.e., ideally, when fitting run-level models, we'd want to smooth the timeseries inputs, and not the output statistical maps—but I'm assuming that for practical reasons, we're currently doing the latter. OTOH, when fitting group-level models where the earliest inputs are subject-level maps, we'd want to smooth the input volumes, and not the output. Not sure what the solution here is... "Smooth the first set of 3D volumes either found or generated during pipeline execution" captures it semantically, but is maybe a bit counterintuitive.

effigies · 2019-05-01T15:55:35Z

The current default is to smooth the BOLD series, not the statistical maps. This PR adds the possibility of moving the smoothing to immediately prior to any stage.

adelavega · 2019-05-01T16:14:41Z

Yes, and in the results I showed you I was smoothing prior to the group level. From what I understand, smoothing prior to the first level would be primarily for noise, whereas prior to the group level would be mostly anatomical heterogeneity.

And if I remember correctly, we decided to do the latter as its much faster for the number of models we're running.

In the future, it might be best if we could feed in pre-smoothed run-level inputs to fitlins, especially if using SUSAN which is quite slow.

tyarkoni · 2019-05-01T16:22:51Z

Oh, good. That's what I would expect intuitively for a default—smooth the very first set of inputs.

effigies · 2019-05-01T16:50:19Z

@adelavega Can you also merge master when you work on this next? There are tests now, so might as well make sure you're not breaking them.

adelavega · 2019-05-01T16:50:55Z

Also to clarify, should the default level be "l1" but it also accepts named levels?

effigies · 2019-05-01T16:52:14Z

Yes, I think so.

effigies

Logic looks good, but nit-picky style changes.

effigies · 2019-05-01T18:33:22Z

fitlins/cli/run.py

-                             "e.g. `--smothing iso:5` will use an isotropic 5mm FWHM kernel")
+                             "Defaults: LEVEL='subject' TYPE='iso';"
+                             "e.g. `--smoothing 5:dataset:iso will use an isotropic 5mm"
+                             "FWHM on subject-level maps, before evaluating the dataset level.")


I had a couple minor comments, in particular the default should be l1 and the metavar should use square brackets to indicate optional components. I also think we skip around a little bit and want to restructure the help text, so here's the whole thing rewritten:

g_prep.add_argument('-s', '--smoothing', action='store', metavar="FWHM[:LEVEL:[TYPE]]", help="Smooth BOLD series with FWHM mm kernel prior to fitting at LEVEL. " "Optional analysis LEVEL (default: l1) may be specified numerically " "(e.g., `l1`) or by name (`run`, `subject`, `session` or `dataset`). " "Optional smoothing TYPE (default: iso) must be one of: `iso` (isotropic). " "e.g., `--smoothing 5:dataset:iso` will perform a 5mm FWHM isotropic " "smoothing on subject-level maps, before evaluating the dataset level.")

Feel free to edit for clarity.

effigies · 2019-05-01T18:47:23Z

fitlins/workflows/base.py

+            if len(smoothing_params) > 2:
+                if smoothing_params[2] != 'iso':
+                    raise ValueError(
+                        f"Unknown smoothing type {smoothing_params[1]}")


Again have a bunch of little comments, so here's a replacement for L63-L80:

smoothing_params = smoothing.split(':', 2) # Convert old style and warn; this should turn into an (informative) error around 0.5.0 if smoothing_params[0] == 'iso': smoothing_params = (smoothing_params[1], 'l1', smoothing_params[0]) warnings.warn( "The format for smoothing arguments has changed. Please use " f"{':'.join(smoothing_params)} instead of {smoothing}.", FutureWarning) # Add defaults to simplify later logic if len(smoothing_params) == 1: smoothing_params.extend(('l1', 'iso')) elif len(smoothing_params) == 2: smoothing_params.append('iso') smoothing_fwhm, smoothing_level, smoothing_type = smoothing_params smoothing_fwhm = float(smoothing_fwhm) if smoothing_type not in ('iso'): raise ValueError(f"Unknown smoothing type {smoothing_type}")

fitlins/workflows/base.py

adelavega · 2019-05-01T19:05:47Z

Okay, pushed those changes. FYI, might be easier in the future if you do something like:
git fetch origin pull/135/head:enh/smoothing
you can then make your changes locally and you should have the power to push to my branch.

effigies · 2019-05-01T19:06:37Z

Cool. Wanted to give you the opportunity to disagree though...

adelavega · 2019-05-01T19:36:02Z

I'm a very agreeable guy...

adelavega · 2019-05-01T19:36:29Z

It would be cool if you could suggest an entire commit though. It's a bit too fussy with the web interface.

effigies · 2019-05-01T19:43:55Z

Sure.

adelavega added 3 commits April 24, 2019 16:11

Add smoothing level to CLI, and pass down

2bacc6d

Pass smoothing kernel to nistats

e808838

Fix typo

305c6d6

adelavega mentioned this pull request Apr 25, 2019

Add group-level smoothing neuroscout/neuroscout-cli#78

Merged

adelavega added 3 commits April 24, 2019 20:05

Don't use pep 517

78e2f39

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

587261d

…/smoothing

Check number of steps for -1 smoothing level

257c08e

effigies mentioned this pull request Apr 29, 2019

ENH: Add version to non-release Docker images. #136

Merged

effigies reviewed Apr 29, 2019

View reviewed changes

Merge branch 'master' into enh/smoothing

e5cca09

effigies reviewed Apr 29, 2019

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

Check out Dockerfile from master

5551cef

adelavega mentioned this pull request Apr 30, 2019

Test pybids 0.9 candidate #138

Closed

adelavega added 2 commits April 30, 2019 17:18

Use step level string instead of integer

dc5c4c5

Merge master

c606ad7

effigies requested changes May 1, 2019

View reviewed changes

fitlins/cli/run.py Outdated Show resolved Hide resolved

fitlins/workflows/base.py Show resolved Hide resolved

fitlins/workflows/base.py Outdated Show resolved Hide resolved

effigies and others added 2 commits May 1, 2019 08:59

Update fitlins/cli/run.py

7762d90

Co-Authored-By: adelavega <aleph4@gmail.com>

Update fitlins/workflows/base.py

db5c6b8

Co-Authored-By: adelavega <aleph4@gmail.com>

adelavega added 3 commits May 1, 2019 12:25

Parse new smoothing options, wit backwards compatability

e1264f0

Check if smoothing_level equals step number

6be8b4f

Merge branch 'master' into enh/smoothing

03aac9e

adelavega requested a review from effigies May 1, 2019 18:34

effigies reviewed May 1, 2019

View reviewed changes

Stylistic improvements to smoothing logic

e6ab269

effigies approved these changes May 1, 2019

View reviewed changes

effigies changed the title ~~ENH: Add smoothing to higher level models~~ ENH: Enable smoothing at any analysis level May 1, 2019

effigies merged commit 20af0e2 into poldracklab:master May 1, 2019

adelavega deleted the enh/smoothing branch May 1, 2019 19:36

effigies mentioned this pull request May 21, 2019

Smoothing does not accept "l<N>" level specification #156

Closed

ENH: Enable smoothing at any analysis level #135

ENH: Enable smoothing at any analysis level #135

Conversation

adelavega commented Apr 24, 2019

adelavega commented Apr 25, 2019 • edited Loading

adelavega commented Apr 25, 2019

effigies Apr 29, 2019

Choose a reason for hiding this comment

effigies Apr 29, 2019

Choose a reason for hiding this comment

effigies commented Apr 29, 2019 • edited Loading

effigies commented Apr 30, 2019

adelavega commented Apr 30, 2019

effigies commented Apr 30, 2019

adelavega commented Apr 30, 2019

adelavega commented Apr 30, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies commented May 1, 2019

adelavega commented May 1, 2019

tyarkoni commented May 1, 2019

effigies commented May 1, 2019

effigies commented May 1, 2019

tyarkoni commented May 1, 2019

adelavega commented May 1, 2019

effigies commented May 1, 2019

effigies commented May 1, 2019

adelavega commented May 1, 2019

tyarkoni commented May 1, 2019

tyarkoni commented May 1, 2019

effigies commented May 1, 2019

adelavega commented May 1, 2019

tyarkoni commented May 1, 2019

effigies commented May 1, 2019

adelavega commented May 1, 2019

effigies commented May 1, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies May 1, 2019

Choose a reason for hiding this comment

effigies May 1, 2019

Choose a reason for hiding this comment

adelavega commented May 1, 2019

effigies commented May 1, 2019

adelavega commented May 1, 2019

adelavega commented May 1, 2019

effigies commented May 1, 2019

adelavega commented Apr 25, 2019 •

edited

Loading

effigies commented Apr 29, 2019 •

edited

Loading