Add 'fslview' displaySpace, and change orientation warnings behavior

CGSchwarzMayo commented 1 year ago

fslview-compatible displaySpace uses pixdim-flip, similar to fslview/fsl coordinate space behavior
Orientation labels are now displayed when in pixdim (scaledVoxels) or pixdim-flip (fslview) spaces, but a warning is displayed that they may be incorrect.
This warning may be suppressed with a new --hideOrientationWarnings

This is similar to https://github.com/pauldmccarthy/fsleyes/issues/84, but for our case we needed pixdim-flip rather than pixdim (scaledVoxels). We are doing a direct migration of workflows from fslview, and some of the images are in old, frozen databases where nii orientations are not consistent and it was never noticed in fslview, but fixing all the affected images at this point would be a burden, so we are seeking an option to use the old fslview-like behavior in this circumstance. We also weren't quite satisfied with the behavior of changing the orientation labels during scaledVoxels, as we know these labels are correct for our images.

From discussing with Paul over email, he suggested that we could display the anatomical orientation labels with a warning that they may be incorrect, and also add a --hideOrientationWarnings that could suppress this warning. I wasn't entirely clear whether this was intended to also suppress the original "displaying images with different orientations" warning. At this point, I only suppressed the new warning, which reverts to the old warning in most of the cases where it would have appeared.

Thanks for your consideration! I am happy to iterate on this. I also wasn't sure whether to keep it as "fslview" space, or change to "fsl" space, as you suggested that fslview's behavior is really the same across all fsl. I can change it if you prefer.

Chris Schwarz Mayo Clinic

pauldmccarthy commented 1 year ago

Hi @CGSchwarzMayo, sorry for the delay - I'll get to this in the next day or so ..

CGSchwarzMayo commented 1 year ago

No problem! It's not such a hurry. Thank you!!

pauldmccarthy commented 1 year ago

Hi @CGSchwarzMayo, I think this looks fine - I'll have a bit of a play to try and wrap my head around the implications, and see if I can make it more robust to being reset when e.g. a second view is opened.

I was also thinking it might be nice to invert the warning logic in the code - i.e. instead of DisplayContext.hideOrientationWarnings defaulting to False, I think it is clearer to have DisplayContext.showOrientationWarnings defaulting to True (but have the command-line flag remaning as --hideOrientationWarnings) - does that sound ok?

One other question - the short form cli flag is currently -now - did you mean to set it to -how? I generally aim to make the short-form flags match the long-form flags as closely as possible (although this is not always possible due to the number of flags).

I'm happy to go ahead and make these changes if they sound good to you (I've already made them on my local copy :wink:).

pauldmccarthy commented 1 year ago

I think the label resetting issue is actually not a problem - it was only relevant to the Xmin/Xmax/etc labels being reset, and was due to some brittle logic in fsleyes.displaycontext.display.DisplayOpts.getLabels:

if opts.transform in ('id', 'pixdim', 'pixdim-flip') and \
   self.displayCtx.displaySpace != refImage:
    # display Xmin/Xmax/etc
else:
    # display L/R/etc

But this issue can be completely resolved by tweaking the logic so that Xmin/Xmax/etc are only displayed when in scaled voxel mode, i.e.:

if self.displayCx.displaySpace == 'scaledVoxel':
    # display Xmin/Xmax/etc
else:
    # display L/R/etc

The former logic is britttle because the DisplayOpts.transform property can be set to pixdim and pixdim-flip in normal circumstances, where we would want to be displaying L/R/etc. With this logic, I was trying to determine whether the L/R/etc orientation labels were valid for all loaded overlays, but I have come to the conclusion that this just isn't possible - whenever we are displaying images with mis-matched FOV/orientation, the labels can only ever be valid for the currently selected overlay.

And, given this, I'm not even sure that an additional warning message in the location panel is necessary - I've come to think that it suffices to retain the current logic, and simply display a warning whenever there is a FOV/orientation mismatch between any pair of overlays (which would already cover the situation I was concerned about with the new fslview mode, when viewing images with different voxel storage orders).

So the behaviour for different displaySpace settings can be much simpler:

Reference image / world/fslview: Display L/R/etc labels for current overlay
scaledVoxel: Display Xmin/Xmax/etc labels

And in all cases, show a warning message about mismatched FOV/orientation, unless all images match (or --hideOrientationWarnings is active).

@CGSchwarzMayo How does all of that sound to you?

CGSchwarzMayo commented 1 year ago

Paul, -now was definitely meant to be -how, so thanks for catching the typo. Fixing the label resetting is great, and it simplifies the code. I have no preference on hide vs show. I'm not sure I agree that "Displaying images with different orientations/fields of view" really conveys to most users that orientation labels may not be correct. To me, that warning indicates that some interpolation has happened, but nothing really about orientation labels. I think if I were you I would leave the "Orientation labels may not be correct" as a separate message, or I would merge the two and use "Displaying images with different orientations/fields of view; orientation labels may not be correct" in all of these cases. I'm still very happy to have this merged either way, though!

I'm happy to make the changes, but if you've already made them in your local copy, I'm even happier to let you proceed and not replicate the work.

Again, thanks for your willingness to consider this feature!

pauldmccarthy commented 1 year ago

This warning covers all bases, but is a little verbose, so I'm not sure if it's ideal - any thoughts?

CGSchwarzMayo commented 1 year ago

I think it's a good approach, but maybe a little editing for concision would make it more friendly to smaller screens/resolutions. How about something like: "Images have differing orientations/sizes; Alignment and/or orientation labels may be incorrect."?

I thought about "FOVs", but maybe not all users know the acronym, so perhaps "sizes" is a good laypersons' term?

A still-shorter version would be: "Images have differing orientations/sizes; They may be misaligned or misoriented." but it may be more confusing than the previous version.

pauldmccarthy commented 1 year ago

I like your first suggestion; the previous version of the message already uses "fields of view", so I think it would work to keep that term in, e.g.: Images have different orientations/fields of view - alignment/orientation labels may be incorrect!

CGSchwarzMayo commented 1 year ago

That works for me, thanks! I'm seeing that this branch has conflicts that must be resolved. That's because you're already in the process of merging it with your changes, and I shouldn't do anything, right?

pauldmccarthy commented 1 year ago

@CGSchwarzMayo Yep, it's been merged (I manage the code at https://git.fmrib.ox.ac.uk/fsl/fsleyes/fsleyes/). I had been under the impression that GitHub was smart enough to detect when commits from a PR have been merged, and to automatically close the PR, but this doesn't seem to be the case. Regardless, this change will be in the next FSLeyes release - thanks again!

CGSchwarzMayo commented 1 year ago

Wonderful. Thank you!

pauldmccarthy / fsleyes

Add 'fslview' displaySpace, and change orientation warnings behavior #117