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

[FIX] don't resample subject-space ROIs unless user provides something #919

Merged
merged 9 commits into from
Nov 29, 2022

Conversation

36000
Copy link
Collaborator

@36000 36000 commented Oct 21, 2022

Currently I think subject-space ROIs are resampled to MNI, which is a bug.
It used to say:
"
If None, the template will be overriden when passed to
an API class."
But I see no evidence that I actually implemented this.

@arokem
Copy link
Collaborator

arokem commented Oct 21, 2022

Do we need to add tests to keep this from breaking?

@36000
Copy link
Collaborator Author

36000 commented Oct 24, 2022

I think we should. do we have some subject-space ROIs for this subject we could use?

@arokem
Copy link
Collaborator

arokem commented Oct 24, 2022

There's a functionally-defined LV1 for the Stanford HARDI dataset here: https://searchworks.stanford.edu/view/ng782rw8378

This notebook shows how it could be downloaded: https://github.com/arokem/visual-white-matter/blob/master/download-data.ipynb

@36000
Copy link
Collaborator Author

36000 commented Nov 8, 2022

What does LV1 mean? lower primary visual cortex? Than should the other ROI between the LGN for the bundle?

@arokem
Copy link
Collaborator

arokem commented Nov 8, 2022

It's left visual cortex. Maybe we can find another ROI in the aparc reduced file that is also in that same collection?

@36000
Copy link
Collaborator Author

36000 commented Nov 11, 2022

@arokem I brought in your changes from the PR in my repository. This is now ready for review

AFQ/tests/test_api.py Outdated Show resolved Hide resolved
@36000
Copy link
Collaborator Author

36000 commented Nov 29, 2022

@arokem this is ready for review / merge

@arokem
Copy link
Collaborator

arokem commented Nov 29, 2022

Looks good. At some point we should also make an example of the subject-space ROI functionality for endpoints, since this is a pretty cool use-case.

@arokem arokem merged commit bed4915 into yeatmanlab:master Nov 29, 2022
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.

2 participants