Add image preprocessing functionality and update seg.py#13
Add image preprocessing functionality and update seg.py#13OmidChaghaneh wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a small “image preprocessing” export module and extends the CeusSeg data object with visualization-related default parameters.
Changes:
- Added
src/image_preprocessing/functions.pyto import/re-export CLAHE and gamma preprocessing functions. - Added visualization default attributes to
CeusSeg(clahe_clip_limit,gamma, width scales,use_philips_ceus). - Introduced/retained empty
__init__.pyfiles to define packages.
Reviewed changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/image_preprocessing/image_preprocessors/init.py | Empty package marker for image_preprocessors. |
| src/image_preprocessing/functions.py | Adds explicit imports for enhance_clahe and enhance_gamma. |
| src/image_preprocessing/init.py | Empty package marker for image_preprocessing. |
| src/data_objs/seg.py | Adds visualization-related default fields on CeusSeg. |
| src/init.py | Empty top-level package marker. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from .image_preprocessors.enhance_clahe import enhance_clahe | ||
| from .image_preprocessors.enhance_gamma import enhance_gamma |
There was a problem hiding this comment.
functions.py only re-exports enhance_clahe/enhance_gamma, but nothing in the repo imports src.image_preprocessing.functions (so this module appears unused) and it omits the other preprocessors in image_preprocessors/. Consider removing it, or making it a deliberate public API by exporting all preprocessors (and/or documenting intended usage) so it doesn't become dead/partial duplication of the dynamic plugin discovery in options.get_im_preproc_funcs().
| self.seg_name: str | ||
| self.seg_mask: np.ndarray | ||
| self.pixdim: List[float] # voxel spacing in mm | ||
| # Visualization defaults | ||
| self.clahe_clip_limit: float = 1.2 |
There was a problem hiding this comment.
CeusSeg.__init__ now initializes several new defaults, but the core fields (seg_name, seg_mask, pixdim) are still only type-annotated and remain unset until assigned elsewhere. For a more consistently-initialized object (and to avoid accidental AttributeError when a CeusSeg() instance is inspected before loaders assign these fields), consider initializing these core attributes to explicit defaults (e.g., None/empty) and updating their type hints accordingly.
| @@ -0,0 +1,2 @@ | |||
| from .image_preprocessors.enhance_clahe import enhance_clahe | |||
There was a problem hiding this comment.
I understand that this addition is likely used to call from the frontend, but such a connection between the backend and frontend would violate the MVC architecture. I'll look through the frontend PR next but would it be possible to remove this file?
There was a problem hiding this comment.
I believe the CLAHE settings have been completely replaced by the P high and P low parameters in Yuanshan’s branch (60-3D-Motion-Compensation).
The main issue is that different developments take time, and merging requests also introduces delays. In the meantime, some features from already developed branches may become outdated, conflict with newer changes, or require rework before integration.
Do you think it would be a good idea to keep this PR open until Yuanshan’s PR is also ready?
| self.seg_name: str | ||
| self.seg_mask: np.ndarray | ||
| self.pixdim: List[float] # voxel spacing in mm | ||
| # Visualization defaults |
There was a problem hiding this comment.
Would you be ok adding these attributes to the UltrasoundImage objects instead of the segmentation one? Conceptually it seems like these visualization attributes would align more in the category of features of the image rather than features of the segmentation
There was a problem hiding this comment.
I’ve made the changes, but as mentioned , we may need to revise this again.
|
If there's nothing requiring these changes that is urgent and separate from Yuanshan's work, I think leaving this PR open for now is a good idea. Ideally, the goal would be that all changes in PRs would just be plugins so it wouldn't make much of a difference when different PRs are merged, but I understand that's not always possible. |
No description provided.