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

Consider renaming StructuralReference interface #612

Open
oesteban opened this issue Feb 5, 2021 · 3 comments
Open

Consider renaming StructuralReference interface #612

oesteban opened this issue Feb 5, 2021 · 3 comments

Comments

@oesteban
Copy link
Member

oesteban commented Feb 5, 2021

Coming out from #610 (comment)

Although, I'm still not sure StructuralReference is a bad name. We are indeed calculating "structural" images (as in, with the maximal structural contrast as possible).

If that argument is convincing, we could close this.

@effigies
Copy link
Member

effigies commented Feb 5, 2021

I think we should upstream the fix directly into RobustTemplate, though maybe triggered by a short_circuit boolean input, in case anybody depends on the failure condition. Note that this interface exists because mri_robust_template errors if only passed one image, not that it successfully does something we don't like or wastes time.

@oesteban
Copy link
Member Author

oesteban commented Feb 5, 2021

Sounds good, a just one comment:

  • Add the short_circuit boolean to the outputs too, for the case someone wants to implement different behaviors depending on whether there were one or more images at the input (asking for a friend).

BTW, we should be careful that these tweaks work well with MapNodes. I just discovered that this tweak -

class _FixN4BiasFieldCorrectionOutputSpec(VanillaN4OutputSpec):
negative_values = traits.Bool(
False,
usedefault=True,
desc="Indicates whether the input was corrected for "
"nonpositive values by adding a constant offset.",
)
class FixN4BiasFieldCorrection(VanillaN4):
"""Checks and fixes for nonpositive values in the input to ``N4BiasFieldCorrection``."""
output_spec = _FixN4BiasFieldCorrectionOutputSpec
def __init__(self, *args, **kwargs):
"""Add a private property to keep the path to the right input."""
self._input_image = None
self._negative_values = False
super(FixN4BiasFieldCorrection, self).__init__(*args, **kwargs)
def _format_arg(self, name, trait_spec, value):
if name == "input_image":
return trait_spec.argstr % self._input_image
return super(FixN4BiasFieldCorrection, self)._format_arg(
name, trait_spec, value
)
def _parse_inputs(self, skip=None):
self._input_image = self.inputs.input_image
# Check intensities
input_nii = nb.load(self.inputs.input_image)
datamin = input_nii.get_fdata().min()
if datamin < 0:
self._input_image = fname_presuffix(
self.inputs.input_image, suffix="_scaled", newpath=os.getcwd()
)
data = input_nii.get_fdata() - datamin
newnii = input_nii.__class__(data, input_nii.affine, input_nii.header)
newnii.to_filename(self._input_image)
self._negative_values = True
return super(FixN4BiasFieldCorrection, self)._parse_inputs(skip=skip)
def _list_outputs(self):
outputs = super(FixN4BiasFieldCorrection, self)._list_outputs()
outputs["negative_values"] = self._negative_values
return outputs
doesn't work.

@effigies
Copy link
Member

effigies commented Feb 5, 2021

  • Add the short_circuit boolean to the outputs too, for the case someone wants to implement different behaviors depending on whether there were one or more images at the input (asking for a friend).

So short_circuit on input is whether to enable short_circuiting and on output indicates whether the short circuit was used? e.g.,

outputs["short_circuit"] = self.inputs.short_circuit and len(self.inputs.in_files) == 1

I feel like I would rather not use the same name for two subtly different concepts, but as long as I understand what you want, we can quibble about naming when we go to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants