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] [DOC] Add matlab to python file conversion functions, add docs for custom tractography integration #599

Merged
merged 26 commits into from
Nov 29, 2020

Conversation

36000
Copy link
Collaborator

@36000 36000 commented Nov 16, 2020

Closes #549

  • Add matlab to pyhton converters from pyAFQ-scripts
  • Add tests for converters
  • Add docs for m2p converter
  • add docs for custom tractography

@36000
Copy link
Collaborator Author

36000 commented Nov 16, 2020

@arokem is there a small matlab file I could use to unit test these new functions?

@36000
Copy link
Collaborator Author

36000 commented Nov 16, 2020

@pep8speaks
Copy link

pep8speaks commented Nov 17, 2020

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

Line 57:63: W291 trailing whitespace

Comment last updated at 2020-11-25 17:32:10 UTC

@36000
Copy link
Collaborator Author

36000 commented Nov 17, 2020

@arokem where should I put the mat files I use for testing here?

@arokem
Copy link
Collaborator

arokem commented Nov 18, 2020

You can put them in a tests/data/ folder, but you'll have to make sure to provide metadata that will tell Python to install the data as part of the package. See: https://github.com/nipy/nibabel/blob/master/setup.cfg#L84

@36000
Copy link
Collaborator Author

36000 commented Nov 18, 2020

I believe thats being used already for some s3 tests. I may have to modify those tests to use that folder.

For example, it looks like test_get_matching_s3_keys expects everything in that folder to be the same as whats in the test bucket.

@richford anything you would like to add to this discussion about how AFQ/tests/data is used?

@36000 36000 force-pushed the conversions branch 3 times, most recently from 9a85f2e to c304dc2 Compare November 18, 2020 22:18
@36000 36000 changed the title [WIP] [ENH] [DOC] Add matlab to python file conversion functions, add docs for custom tractography integration [ENH] [DOC] Add matlab to python file conversion functions, add docs for custom tractography integration Nov 18, 2020
@36000
Copy link
Collaborator Author

36000 commented Nov 18, 2020

I ended up moving the mock s3 data into a "mocks3" folder in data to separate it from the rest.

Anyways, this is ready for review / merge @arokem .

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.

Overall, looks good! Just a few minor comments/suggestions.

docs/source/usage/converter.rst Outdated Show resolved Hide resolved
docs/source/usage/converter.rst Outdated Show resolved Hide resolved
docs/source/usage/converter.rst Show resolved Hide resolved
docs/source/usage/converter.rst Show resolved Hide resolved
AFQ/utils/conversion.py Outdated Show resolved Hide resolved
AFQ/utils/conversion.py Outdated Show resolved Hide resolved
AFQ/utils/conversion.py Outdated Show resolved Hide resolved
AFQ/utils/conversion.py Outdated Show resolved Hide resolved
docs/source/usage/converter.rst Outdated Show resolved Hide resolved
docs/source/usage/converter.rst Outdated Show resolved Hide resolved
36000 and others added 8 commits November 25, 2020 09:24
Co-authored-by: Ariel Rokem <arokem@gmail.com>
Co-authored-by: Ariel Rokem <arokem@gmail.com>
Co-authored-by: Ariel Rokem <arokem@gmail.com>
Co-authored-by: Ariel Rokem <arokem@gmail.com>
Co-authored-by: Ariel Rokem <arokem@gmail.com>
Co-authored-by: Ariel Rokem <arokem@gmail.com>
@36000
Copy link
Collaborator Author

36000 commented Nov 25, 2020

@arokem I tried using squeeze_me but I got confused by what it was returning. The current system works for the files I have encountered, so I don't know if it would be useful to re-figure it out with squeeze_me.

Other than that, this is ready for review / merge.

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

Add Recommended Usage for Converting from MATLAB AFQ to pyAFQ
3 participants