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

Segmentation Refactoring and Other Updates #136

Merged
merged 43 commits into from
Aug 31, 2019

Conversation

36000
Copy link
Collaborator

@36000 36000 commented Aug 1, 2019

In the process of putting segmentation into its own class. Added logging, option to resample. trying numba.

@pep8speaks
Copy link

pep8speaks commented Aug 1, 2019

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 2019-08-29 22:23:55 UTC

@36000
Copy link
Collaborator Author

36000 commented Aug 10, 2019

@arokem Take a look at the current Segmentation class / file. This should be ready to merge.

Copy link
Collaborator

@arokem arokem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are a few comments.

The main comment I have about this is the comment I made in the api module: we want to make sure that the API for the segmentation functions is the same, so that we can reduce code duplication in api and lay the ground for more flexibility going forward.

Let's discuss when we meet later today.

AFQ/segmentation.py Outdated Show resolved Hide resolved
AFQ/segmentation.py Outdated Show resolved Hide resolved
AFQ/segmentation.py Outdated Show resolved Hide resolved
AFQ/segmentation.py Show resolved Hide resolved
AFQ/segmentation.py Outdated Show resolved Hide resolved
AFQ/api.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@arokem arokem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more things to clarify/clean/test...

AFQ/segmentation.py Outdated Show resolved Hide resolved
AFQ/segmentation.py Show resolved Hide resolved
AFQ/segmentation.py Show resolved Hide resolved
AFQ/segmentation.py Show resolved Hide resolved
AFQ/segmentation.py Show resolved Hide resolved
AFQ/segmentation.py Outdated Show resolved Hide resolved
'bundles',
'CST_R.trk'))
'bundles',
'CST_R.trk'))


# def test_AFQ_data_recobundles():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably try to add a test using the 'reco' algorithm, but we probably need to remove this one -- it takes forever to run.

AFQ/tractography.py Outdated Show resolved Hide resolved
examples/plot_recobundles.py Outdated Show resolved Hide resolved
examples/plot_tract_profile.py Outdated Show resolved Hide resolved
@36000
Copy link
Collaborator Author

36000 commented Aug 29, 2019

@arokem , the only thing this needs is a reco API test. I won't be able to finish it today, but I think I resolved all the other comments if you want to look at them before you leave.

@arokem
Copy link
Collaborator

arokem commented Aug 31, 2019

Yeah - this all looks good. Let's go ahead and merge this and then add an issue to remind us to go back and add that test.

@arokem arokem merged commit a51ab36 into yeatmanlab:master Aug 31, 2019
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.

3 participants