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

Enforce use of display_viewer_syntax in four scripts using hardcoded fsleyes commands #3895

Merged
merged 7 commits into from Oct 6, 2022

Conversation

joshuacwnewton
Copy link
Member

Description

Most SCT scripts use a function called display_viewer_syntax at the end of processing. This function searches for the presence of image viewing software like fsleyes or itksnap, then outputs commands that let the user open output files in those viewers.

However, some scripts still hardcode a command for fsleyes/fslview. So, this PR replaces those commands with display_viewer_syntax.

((NB: The isct_convert_binary_to_trilinear script is looking a bit neglected. I've made sure it works, because it's listed in our setup.py, and thus is available to our users. That said, the script doesn't use argparse, and its main function isn't properly exposed, so we don't currently test it.

We might want to reflect on whether or not these isct_ scripts are worth maintaining. If they are, we should fix them up and turn them into a properly formatted + tested sct_ scripts. If they aren't, then we should move them to dev/.))

Linked issues

Fixes #3883.

This one isn't an exact 1:1 translation, as before, the old command simply put `fname_mask` without any colormap/opacity information.

The default FSLeyes settings, though, are grayscale ('gray') and opaque ('1.0'), so specifying these values will produce identical results in the viewer.
…mand

This one isn't an exact 1:1 translation, as before, the old command simply put `arguments.i` without any colormap/opacity information.

The default FSLeyes settings, though, are grayscale ('gray') and opaque ('1.0'), so specifying these values will produce identical results in the viewer.

Additionally, because `fname_out_lst` has variable length, we also set variable-length colormap and opacity lists, too.
This one is very straightforward, with no colormaps/opacities needed.
sct_resample expected a `.nii` file, but sct_smooth_spinalcord
generated a `.nii.gz` file, so update the internal commands
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, including figuring out the missing grayscale and opacity parameters.

We might want to reflect on whether or not these isct_ scripts are worth maintaining. If they are, we should fix them up and turn them into a properly formatted + tested sct_ scripts. If they aren't, then we should move them to dev/.))

Good question, this is worth discussing at the next dev meeting. In particular, your bugfix of the isct_convert_binary_to_trilinear means that this script has been completely broken for quite some time, so I can't imagine it gets much use.

Out of scope for this PR: I'm a bit surprised that sct_smooth_spinalcord produces a .nii.gz even when the input is a plain .nii file. Is this intended? My impression from the existence of Image.add_suffix() is that we usually try to match input and output formats. As a further aside, Image.add_suffix() uses an almost-but-not-quite-identical implementation of utils.sys.extract_fname(), which puzzles me.

@joshuacwnewton joshuacwnewton merged commit afdae7e into master Oct 6, 2022
@joshuacwnewton joshuacwnewton deleted the jn/3883-enforce-use-of-display-viewer-syntax branch October 6, 2022 16:04
@joshuacwnewton joshuacwnewton added the enhancement category: improves performance/results of an existing feature label Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement category: improves performance/results of an existing feature sct_analyze_lesion context: sct_analyze_texture context: sct_denoising_onlm context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 SCT scripts are hardcoding FSLeyes commands, rather than using display_viewer_syntax
2 participants