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

Generating a wm mask out of multiple scalars #330

Merged
merged 19 commits into from
Aug 10, 2020

Conversation

36000
Copy link
Collaborator

@36000 36000 commented Aug 5, 2020

No description provided.

@pep8speaks
Copy link

pep8speaks commented Aug 5, 2020

Hello @36000! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-10 15:40:14 UTC

@36000 36000 changed the title Generating a wm mask out of multiple scalars [WIP] Generating a wm mask out of multiple scalars Aug 5, 2020
@36000 36000 added this to the Version 0.4 milestone Aug 5, 2020
@36000 36000 changed the title [WIP] Generating a wm mask out of multiple scalars Generating a wm mask out of multiple scalars Aug 6, 2020
AFQ/api.py Outdated Show resolved Hide resolved
AFQ/api.py Outdated Show resolved Hide resolved
@36000
Copy link
Collaborator Author

36000 commented Aug 6, 2020

I think the documentation error here was just a connection problem.

@arokem
Copy link
Collaborator

arokem commented Aug 6, 2020

I have a button that allows me to rerun builds. Maybe you see that one too?

@36000 36000 changed the title Generating a wm mask out of multiple scalars [WIP] Generating a wm mask out of multiple scalars Aug 7, 2020
@arokem
Copy link
Collaborator

arokem commented Aug 7, 2020

Is this ready? The PR still says "WIP", so I figured I'd ask before merging.

@36000
Copy link
Collaborator Author

36000 commented Aug 7, 2020

Nope! Right now I am using it on the prek data and I am finding that there is still overlap between the resulting wm mask and high values in MD. I marked this as WIP when I realized this was happening! I am investigating this bug and I will let you know when I think this works as intended.

@36000
Copy link
Collaborator Author

36000 commented Aug 7, 2020

I think I have located the problem but I would like it if @arokem could make sure my understanding of this is correct.
We use this line on our wm_masks:
wm_mask = binary_dilation(wm_mask) > 0 (

wm_mask = binary_dilation(wm_mask) > 0
)
When we were getting wm masks from segmentation files / an arbitrary FA cutoff this made sure we were not being too strict. But now that we are trying to make specific masks we should not be doing this.
Should we do this for seg files but not wm masks based off of scalars? Or should it be based on user input?

P.S. Some extreme tunnel vision that I did not find this earlier...

@arokem
Copy link
Collaborator

arokem commented Aug 7, 2020

Oh - good spot! I think that this was a precaution against overly conservative masks, but I think that's really not an issue anymore. Given everything we've seen in the last couple of weeks, I am starting to think that maybe there is never a reason to use binary dilation here. And as you've demonstrated, it can lead to some puzzling outcomes. Maybe we should remove it altogether?

@36000 36000 force-pushed the wm_mask-dict branch 4 times, most recently from c5435fa to 93e8380 Compare August 9, 2020 18:48
@36000 36000 changed the title [WIP] Generating a wm mask out of multiple scalars Generating a wm mask out of multiple scalars Aug 9, 2020
@36000
Copy link
Collaborator Author

36000 commented Aug 9, 2020

Because of this interpolation trickiness, I may just revert to using an FA mask for tracking to replicate mAFQ, but then mask out any nodes with high MD after profiling.

@arokem arokem merged commit 1f22983 into yeatmanlab:master Aug 10, 2020
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