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

WIP - Update scilpy & tractometry #2

Merged
merged 14 commits into from
Oct 21, 2020
Merged

Conversation

frheault
Copy link
Member

@frheault frheault commented Jul 2, 2020

Adaptation to the recent modification in scilpy

@frheault frheault changed the title Update scilpy & tractometry WIP - Update scilpy & tractometry Jul 2, 2020
@arnaudbore arnaudbore requested a review from alexjoa September 9, 2020 17:55
main.nf Outdated Show resolved Hide resolved
Copy link
Contributor

@GuillaumeTh GuillaumeTh left a comment

Choose a reason for hiding this comment

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

Don't tested yet. A small comment about naming convention. Don't tested yet. @frheault Could you give me the path of the singularity used for this PR or at least scilpy commit number ?

main.nf Outdated Show resolved Hide resolved
Copy link
Contributor

@GuillaumeTh GuillaumeTh left a comment

Choose a reason for hiding this comment

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

The pipeline crash. Maybe a too old version of the models ?

File "/usr/local/lib/python3.7/dist-packages/scilpy-0.2.dev0-py3.7-linux-x86_64.egg/scilpy/segment/voting_scheme.py", line 73, in _init_bundles_tag
      for key in self.config.keys():
  AttributeError: 'list' object has no attribute 'keys'

main.nf Show resolved Hide resolved
Copy link
Contributor

@GuillaumeTh GuillaumeTh left a comment

Choose a reason for hiding this comment

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

Worked well with the atlas you gave me. Please fix the comments and I will approve after.

@frheault
Copy link
Member Author

frheault commented Oct 2, 2020

@GuillaumeTh Done

Copy link
Contributor

@mdesco mdesco left a comment

Choose a reason for hiding this comment

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

Aside from my comestic suggestions. This works and was tested with and without the singularity tractoflow-scilpy-rc2

@@ -11,7 +11,7 @@ DESCRIPTION

[root]
├── S1
│ ├── *anat.nii.gz
│ ├── *fa.nii.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really need to be an FA?
For instance, in my test case, I have b0 and I believe it should work, no?

Can you add something in the help about that please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also,

  1. Can you provide a link to your zenodo URL please
  2. Can you add a centroids directory or download on the zenodo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, a lot of our flows use --root, others use --input. Can we harmonize this?

--input or --root?

Copy link
Contributor

Choose a reason for hiding this comment

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

--input

Copy link
Member Author

Choose a reason for hiding this comment

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

It should work with anything, but I want to leave *fa.nii.gz as an input name

Copy link
Contributor

@GuillaumeTh GuillaumeTh left a comment

Choose a reason for hiding this comment

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

Tested and worked well. Good job ! 🍻

@GuillaumeTh GuillaumeTh merged commit 0b68bad into scilus:master Oct 21, 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.

4 participants