Improve handling of segmentation LUTs#51
Conversation
| def _preprocess_lut(self, lut: pd.DataFrame) -> pd.DataFrame: | ||
| # dummy function for subclasses to override if they need to preprocess the LUT after loading | ||
| return lut |
There was a problem hiding this comment.
Do you think we should like to send in a different lut than the one that is saved on the instance (i.e self.lut), if not we can probably change the signature of the method to
def _preprocess_lut(self) -> None: ...and modify self.lut inplace.
There was a problem hiding this comment.
Another option could be that you can pass a standalone function with the signature
def preprocess_lut(lut: pd.DataFrame) -> pd.DataFrame: ...as an argument to the class initializer. That would also make it more customisable.
There was a problem hiding this comment.
I think probably the first option is better. In the second case you may get unexpected behavior from your lut (like how it is indexed etc)
| if lut_path is None and seg_path.with_suffix(".csv").exists(): | ||
| lut_path = seg_path.with_suffix(".csv") |
There was a problem hiding this comment.
I am OK with using .csv suffix, but perhaps the user can provide this suffix as an argument instead, so that it also works with .json. Perhaps we want to support other formats in the future as well?
There was a problem hiding this comment.
I don't see why not. My main goal here was to be consistent with freesurfer (i.e make it easy to use a lut you can directly load the lut into freeview). I struggled a lot with .json. I believe the code assumed .json previously
| self.lut = lut.set_index(label_column) | ||
| self.label_name = label_column | ||
| else: | ||
| self.label_name = "label" if "label" in self.lut.columns else self.lut.columns[0] |
There was a problem hiding this comment.
So use "label" if that is in one of the columns, otherwise use the first column. Is "use the current index" the same? (ref docstrings)
| # Get evenly spaced values between 0 and 1 based on the number of labels | ||
| color_indices = np.linspace(0, 0.95, len(labels)) | ||
| # Sample a colormap | ||
| rgb_float = plt.get_cmap(cmap)(color_indices) |
There was a problem hiding this comment.
So this is the only reason we need to bring in matplotlib as a dependency. I am on board with that, but it is a heavy dependency so might consider adding it as an optional dependency and move the import inside the function call.
There was a problem hiding this comment.
I will modify this to use colorsys by default and matplotlib optional
| "scikit-image", | ||
| "pydicom", | ||
| "dcm2niix", | ||
| "matplotlib>=3.10.9", |
There was a problem hiding this comment.
We should not set a lower bound, unless we need to. The will make the library less flexible and harder to install.
There was a problem hiding this comment.
This was uv, forgot to check this!
Co-authored-by: Henrik Finsberg <henrikfinsberg@hotmail.com>
|
Looks good to me. Thanks @Erasdna |
2 participants
Changed some of how luts are handled in the segmentation classes:
Also modified some statistics computations. Using pandas is faster when computing multiple statistics I believe.