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

Append multiple clusters into one vtp file #48

Merged
merged 8 commits into from
Aug 4, 2016

Conversation

zhangfanmark
Copy link
Member

Hi Lauren,

Please see the script for appending the clusters. It takes the input e.g. 1,2,3 and generates a 'cluster_appended_1_2_3.vtp'. The point scalar 'cluster_idx' is added to view the fibers from different subjects.

Regards,
Fan

@ljod
Copy link
Member

ljod commented Aug 3, 2016

Hi Fan. Thanks for writing this. Could you use the standard argparse list format for multiple arguments instead of the comma-separated no spaces string? I think this will be less error-prone for users. See first example here for nargs +:

https://docs.python.org/3/library/argparse.html

@zhangfanmark
Copy link
Member Author

Hi Lauren, please see the updates. Thanks!

Regards,
Fan

@ljod
Copy link
Member

ljod commented Aug 3, 2016

Nice. Two thoughts:

This line is duplicated so the array is added twice
pd_cluster_point = pd_cluster.GetPointData().AddArray(vtk_array)

Also at the end please don't write patients because this will be several clusters from the same subject--perhaps the helpful printout can say something about cluster number not patient?

@zhangfanmark
Copy link
Member Author

Hi Lauren,

Sorry I forgot this commit. Do you think it is okay now? Thanks!

Regards,
Fan

@ljod
Copy link
Member

ljod commented Aug 4, 2016

Sure it must be ok now.

On Thu, Aug 4, 2016 at 6:54 PM -0400, "zhangfanmark" <notifications@github.commailto:notifications@github.com> wrote:

Hi Lauren,

Sorry I forgot this commit. Do you think it is okay now? Thanks!

Regards,
Fan


You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com//pull/48#issuecomment-237709036, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEgM973D7n_-oK-_WtJGYQgDzPzOa5chks5qcm22gaJpZM4JcFJp.

The information in this e-mail is intended only for the person to whom it is
addressed. If you believe this e-mail was sent to you in error and the e-mail
contains patient information, please contact the Partners Compliance HelpLine at
http://www.partners.org/complianceline . If the e-mail was sent to you in error
but does not contain patient information, please contact the sender and properly
dispose of the e-mail.

@ljod ljod merged commit 4875bcd into SlicerDMRI:master Aug 4, 2016
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

2 participants