CRITICAL BUG FIX #405
CRITICAL BUG FIX #405
Conversation
Fixes windowing for image fusion deleted if statement and just made custom levels for init3 previously it was suing the dicom levels which didnt work with vtk fixed critical bug with auto segment for windows changed it to string as it was not passing a string to the rtstruct
Nowhere4Nothing
requested review from
Kahreiru,
Timmy-Codes,
mattbrgn and
sjswerdloff
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR overhauls the VTK image fusion window/level logic by replacing dynamic DICOM-based lookups with static presets, simplifies fusion_views handling, corrects argument ordering in the mouse release handler, and fixes a type error in RTstruct saving. Sequence diagram for mouse release event and window/level updatesequenceDiagram
actor User
participant WindowingSlider
participant windowing_model_direct
participant FusionView
User->>WindowingSlider: mouse_release(event)
WindowingSlider->>windowing_model_direct: windowing_model_direct(window, level, send)
windowing_model_direct->>FusionView: set_window_level(window, level)
windowing_model_direct->>FusionView: update_color_overlay()
Class diagram for updated Windowing model and fusion_views handlingclassDiagram
class WindowingSlider {
+fusion_views: list
+mouse_release(event)
}
class windowing_model {
+windowing_model(text, init)
}
class windowing_model_direct {
+windowing_model_direct(window, level, init, fixed_image_array=None)
}
WindowingSlider --> windowing_model_direct : calls
windowing_model --> windowing_model_direct : uses
Class diagram for NiftiToRtstructConverter save method fixclassDiagram
class NiftiToRtstructConverter {
+nifti_to_rtstruct_conversion(nifti_path, dicom_path, output_path, ...)
}
class RTStruct {
+save(path: str)
}
NiftiToRtstructConverter --> RTStruct : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Reintroduce or at least log when dynamic presets from MovingDictContainer aren’t used so that missing user configurations aren’t silently dropped.
- Assigning fusion_views to an empty list by default may change downstream behavior—verify that components handle [] the same way they did None and adjust accordingly.
- Make sure the swapped window/level argument order in windowing_model_direct is applied consistently across all usages to avoid signature mismatches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Reintroduce or at least log when dynamic presets from MovingDictContainer aren’t used so that missing user configurations aren’t silently dropped.
- Assigning fusion_views to an empty list by default may change downstream behavior—verify that components handle [] the same way they did None and adjust accordingly.
- Make sure the swapped window/level argument order in windowing_model_direct is applied consistently across all usages to avoid signature mismatches.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sjswerdloff
left a comment
sjswerdloff
left a comment
There was a problem hiding this comment.
I am very hesitant about this PR given the change to the parameter order of the invocation of windowing_model_direct. I'll approve it because it's marked as a critical bug fix, but I would really like to have attestation that all window/leveling is working throughout the application and not just the area being investigated.
| """ | ||
| Function triggered when a window is selected from the menu. | ||
| :param text: The name of the window selected. | ||
| :param init: list of bool to determine which views are chosen |
There was a problem hiding this comment.
better to use a dictionary with name/value pairs with the name (key) representing what it is that is being specified.
Mark this as a new issue (area for improvement).
src/Model/Windowing.py
Outdated
| "Lung": [1600, -600], # Lung, covers -1400 to 200 HU | ||
| "Bone": [2000, 300], # Bone, covers -700 to 1300 HU | ||
| "Brain": [80, 40], # Brain, covers 0 to 80 HU | ||
| "Soft Tissue": [440, 40], # Alias for Normal |
There was a problem hiding this comment.
If "Soft Tissue" is an alias for "Normal" why are the values different?
There was a problem hiding this comment.
comment doesnt need to be there, I changed it to closer to what it is in the dicom part and didnt remove the comment my bad
Soft Tissue,CT,440,800
edit: normal window in dicom view is 400 / 800 as in moving_model.py line 50 ill change all the normals to 400/40
| def windowing_model(text, init): | ||
| """ | ||
| Function triggered when a window is selected from the menu. | ||
| :param text: The name of the window selected. |
There was a problem hiding this comment.
why not just call the variable windowing_name or something like that.
Yes, it's the text that is the key to a dictionary, but that's a very abstract way of specifying what a variable is. I'm a strong believer in naming variables according to what they represent (so that the code can be read by a human and understood. See "literate programming". https://en.wikipedia.org/wiki/Literate_programming
While python isn't exactly what Donald Knuth had in mind, writing your program so that it can be read as if it was prose makes it easier to understand.
There was a problem hiding this comment.
because it was added in 2021 and i didnt want to change it just in case it broke something
| # Apply the window/level changes using the model | ||
| with wait_cursor(): | ||
| windowing_model_direct(level, window, send) | ||
| windowing_model_direct(window, level, send) |
There was a problem hiding this comment.
I'd need to see the history of this to understand why this is being reversed in terms of window/level.
This seems very risky, and may imply that the verification testing is not including adequate regression testing.
There was a problem hiding this comment.
in a previous PR
#386
i mistook you thinking u said swap them so i did. I should of asked you to clarify. My bad
There was a problem hiding this comment.
in a previous PR #386 i mistook you thinking u said swap them so i did. I should of asked you to clarify
OK. "Almost looks like" should be interpreted as "go investigate and trace to the bottom to make sure that these are being passed in correctly".
Changed windowing level to match the dicom 400/40 changed the comments to match
Update windowing to get the tests to pass now they are fixed
| # Apply the window/level changes using the model | ||
| with wait_cursor(): | ||
| windowing_model_direct(level, window, send) | ||
| windowing_model_direct(window, level, send) |
There was a problem hiding this comment.
in a previous PR #386 i mistook you thinking u said swap them so i did. I should of asked you to clarify
OK. "Almost looks like" should be interpreted as "go investigate and trace to the bottom to make sure that these are being passed in correctly".
| } | ||
| windowing_limits = custom_presets.get(text, [400, 40]) | ||
| custom_presets = { | ||
| "Normal": [400, 40], # Normal levels (typical soft tissue window) |
There was a problem hiding this comment.
Is Normal supposed to be the same as Soft Tissue or not? [400,40] != [440,40]
If it's not supposed to be the same thing, then explain why Normal is almost the same but not quite.
| "Head and Neck": [275, 40], # Covers -147.5 to 227.5 HU | ||
| } | ||
| windowing_limits = custom_presets.get(text, [400, 40]) | ||
| custom_presets = { |
There was a problem hiding this comment.
This is essentially a fixed dictionary, unless it's being updated elsewhere.
And for these hardcoded settings, it is a fixed dictionary:
WINDOW_LEVEL_PRESETS = { "LUNG": [1600, -600], ...}
DEFAULT_PRESET = WINDOW_LEVEL_PRESETS["NORMAL"]
And then just do the lookup
windowing_limits = WINDOW_LEVEL_PRESETS.get(text, DEFAULT_PRESET)
| "Bone": [2000, 300], # Bone, covers -700 to 1300 HU | ||
| "Brain": [80, 40], # Brain, covers 0 to 80 HU | ||
| "Soft Tissue": [440, 40], # Alias for Normal | ||
| "Soft Tissue": [440, 40], # Soft tissue, covers -180 to 260 HU (muscle, fat, organs) |
There was a problem hiding this comment.
So the window is the difference between the two HU values (upper - lower).
And the window centre in HU is (upper+lower)/2.
It might be better to have a dictionary of HOUNSFIELD_UPPER_LOWER_BOUNDS so the physics is clear regarding what is going on, and then calculate (in to a constant dictionary) for CUSTOM_PRESETS from that.
4 participants
Fixes windowing for image fusion
deleted if statement and just made custom levels for init3 previously it was suing the dicom levels which didnt work with vtk
fixed critical bug with auto segment for windows changed it to string as it was not passing a string to the rtstruct, if not added it crashes auto segment at end for windows
Summary by Sourcery
Fix critical bugs in image fusion windowing and RTSTRUCT conversion on Windows by simplifying windowing logic, correcting function call arguments, initializing fusion views, and casting save paths to string.
Bug Fixes: