Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Run auto model by default #26

Merged
merged 7 commits into from May 6, 2018

Conversation

effigies
Copy link
Collaborator

Closes #24

@effigies
Copy link
Collaborator Author

So it looks like auto_model creates a battery of models. That's going to make the current nipype-ified architecture a little weird. Will need to see if it makes more sense to change automodel or the interface.

@tyarkoni
Copy link
Collaborator

Can you elaborate? What's the fitlins-optimal approach, and what is it currently doing?

@effigies
Copy link
Collaborator Author

effigies commented Apr 30, 2018 via email

@tyarkoni
Copy link
Collaborator

tyarkoni commented Apr 30, 2018

Ohhh, I see. Sorry, I'd forgotten auto_model returns a list of models. I think the reason for that is that there can be multiple available tasks, and since each run can only have one task value in BIDS, it would be a bit weird to return a single object that in effect contains multiple models that require different sets of inputs.

What I think we could do though that might work for your needs is allow the user to pass keyword selectors to auto_model, and in the event that the user passes just one task, a single model object is returned. Would that work for you? @Shotgunosine, any objections to/thoughts on this proposal?

@effigies
Copy link
Collaborator Author

Well, the point here would be to auto run, so I wouldn't assume enough selectors from users to get a unique model, as currently built. If we can't have a single model that handles all the tasks, then I should handle lists over here.

@tyarkoni
Copy link
Collaborator

I think if you're going to go down that road (which seems reasonable), you'll need to implicitly loop over all detected tasks and run the whole fitlins pipeline for each one... so it seems like a more general problem than what's happening in auto_model.

@Shotgunosine
Copy link
Collaborator

I guess an alternative would be to have a flag on auto_model that generates a single model for all tasks. I'm assuming you mean that we'd concatenate the runs for all the tasks together and fit a single model across all of those tasks. Off the top of my head I can't think of a reason that wouldn't be doable in automodel. I'll leave it to @tyarkoni to comment on how BIDS appropriate that would be.

@effigies
Copy link
Collaborator Author

I was thinking of combining in the sense of building a model as a series of independent chains (i.e. block n might depend on blocks 1..n-1, but doesn't need to depend on all). The goal would not be to change the semantics; just the organization.

@tyarkoni
Copy link
Collaborator

@Shotgunosine I think in principle the approach you suggest already works under the current spec. But fitting one big model across multiple tasks is something of an edge case, and has a different meaning (and will produce different results) from the typical scenario, where one is more likely to have several different models, each of which needs to be fit to a different task.

@effigies I may be misunderstanding what you're saying, but the models coming back from auto_model always only apply to level='run'. The list of models you get back would presumably have to be run completely in parallel; I don't think there can be any dependency between the elements of that list.

@effigies
Copy link
Collaborator Author

@tyarkoni I agree each element of the list should be independent. I was suggesting there might be (I don't know) a way of combining into a single model where the blocks are independent.

@effigies
Copy link
Collaborator Author

Just pushed a proposed refactoring, which always assumes multiple independent models. I suspect flattening inside the node will probably end up producing an untenable situation when we try to recombine at the second level... It's possible that what we want is to just load the models (whether they're json files or auto-generated) and construct entirely independent pipelines for each.

@tyarkoni
Copy link
Collaborator

I can't think of any situation in which a user would want/need to recombine models at the second level in a way that can be easily automated. I think we can safely construct completely independent pipelines for each task (beginning with the subsetting of the input image list based on the task).

@effigies
Copy link
Collaborator Author

effigies commented May 3, 2018

Think this is ready, by the way. Reviews welcome. Will merge tomorrow unless somebody requests more time to review.

@effigies
Copy link
Collaborator Author

effigies commented May 6, 2018

Guess I forgot.

@effigies effigies merged commit c7f19f3 into poldracklab:master May 6, 2018
@effigies effigies deleted the enh/default_model branch May 6, 2018 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants