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

FIX Correctly handle NaNs in VertexRGB and VolumeRGB #458

Merged
merged 7 commits into from
Jul 13, 2022

Conversation

mvdoc
Copy link
Contributor

@mvdoc mvdoc commented Jul 12, 2022

When creating a new VertexRGB object with vertices containing NaNs, the values of those vertices would be cast to uint8 and transformed to 0. However, the alpha value would be set to 1, causing the Nan vertices to appear black. This PR fixes this issue by automatically setting a correct alpha of 0 for NaN values. It partially addresses #455.

Minimal example:

import cortex
import numpy as np
import matplotlib.pyplot as plt

# get an roi to mask out
roi = cortex.get_roi_verts("fsaverage", roi="IPS0")["IPS0"]

# Make a VertexRGB object
c = np.linspace(0, 1, 327684)
rgb = np.array([c, np.zeros_like(c), np.zeros_like(c)])
rgb[:, roi] = np.nan
vtx = cortex.VertexRGB(*rgb, "fsaverage")

cortex.webgl.show(vtx)
cortex.quickshow(vtx, with_colorbar=False, with_curvature=True);plt.show()

Before:
image
image

After:
image
image

@TomDLT
Copy link
Contributor

TomDLT commented Jul 13, 2022

This is definitely better than before.

Two nitpicks:

  • if self.red is updated after initialization, self.alpha is not updated.
  • if self.alpha is given as an array, it ignores NaNs in the data.

We could address both issues with the slightly more complicated solution:

    def __init__(...):
        ...
        self.alpha = alpha

    @property
    def alpha(self):
        """Compute alpha transparency"""
        alpha = self._alpha
        if alpha is None:
            alpha = np.ones(self.red.vertices.shape)
            alpha = Vertex(alpha, self.red.subject, vmin=0, vmax=1)
        if not isinstance(alpha, Vertex):
            alpha = Vertex(alpha, self.red.subject)
        
        rgb = np.array([self.red.data, self.green.data, self.blue.data])
        mask = np.isnan(rgb).any(axis=0)
        alpha._data[mask] = alpha.vmin
        return alpha

    @alpha.setter
    def alpha(self, alpha):
        self._alpha = alpha

@mvdoc
Copy link
Contributor Author

mvdoc commented Jul 13, 2022

Done. I copied your suggestion as it was, with a small change:

# alpha._data[mask] = alpha.vmin
alpha._data[..., mask] = alpha.vmin

If alpha is set to None, the data of the alpha channel for a VertexRGB object is 2D. If instead a Vertex object is explicitly passed as alpha, then the data of the alpha channel is 1D.

@mvdoc
Copy link
Contributor Author

mvdoc commented Jul 13, 2022

Done. I copied your suggestion as it was, with a small change:

# alpha._data[mask] = alpha.vmin
alpha._data[..., mask] = alpha.vmin

If alpha is set to None, the data of the alpha channel for a VertexRGB object is 2D. If instead a Vertex object is explicitly passed as alpha, then the data of the alpha channel is 1D.

Nevermind. Vertex.vertices always returns a 2D array, and that's why alpha became a 2D array when initialized with None. Now it should be OK.

@TomDLT
Copy link
Contributor

TomDLT commented Jul 13, 2022

Thanks for adding it to VolumeRGB too. Then I think we should refactor it into DataviewRGB, to avoid code duplication. It seems that self.red.vertices and self.red.volume could be merged with self.red.data.

edit: Nevermind, creating a Volume/Vertex for alpha might be hard to merge into one code. We might as well duplicate the code.

@mvdoc
Copy link
Contributor Author

mvdoc commented Jul 13, 2022

Then I think we should refactor it into DataviewRGB, to avoid code duplication. It seems that self.red.vertices and self.red.volume could be merged with self.red.data.

Yeah, I started doing that, but then realized that DataviewRGB should be refactored heavily to make it a bit more robust. Now DataviewRGB tries to access properties that do not exist yet (e.g., .red, which only exists for the subclasses). Also, as you discovered there is not a lot of consistency between VertexRGB and VolumeRGB, so I had to duplicate the code...

I also had to modify VolumeRGB.color_voxels. It was casting nans to 0 without setting an appropriate alpha. I'll merge once I make sure that it is working...

@mvdoc mvdoc changed the title FIX Correctly handle NaNs in VertexRGB FIX Correctly handle NaNs in VertexRGB and VolumeRGB Jul 13, 2022
@mvdoc mvdoc merged commit 82e5781 into gallantlab:main Jul 13, 2022
@mvdoc mvdoc deleted the fix/vertexRGBnans branch July 13, 2022 19:01
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