-
Notifications
You must be signed in to change notification settings - Fork 1
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
code fixes issues #53, #49, #33, #59 - GUI update #56
base: main
Are you sure you want to change the base?
Conversation
…format yml based on format written by yaml.dump
…file, apply template default values
…en the current config and the output folder simplification : If this is a new configuration of SlicerCART (new or template), require empty output folder. Otherwise, we've taken the exact configuration and are working in the same directory as the selected output folder. In Issue #30, it can be revisited whether we would like to allow modifications to the configuration after having started the data collection (segmentation or classification) and which of these modifications should be allowed.
Good work Kuan! Since this new parameter is only pertinent if the segmentation task is requested, the checkbox could even be disabled when the 'Segmentation' task checkbox is not checked. What do you think? |
thanks! |
…d segmentation options now start the timer for this option
…the future for visibility
…r treats it as scriptable loadable module
…e - coordinates of control points and lengths
Regarding issue #37 , I was wondering if saving measurements into the csv file would be enough? Or should we load them into each segmentation? |
work in progress. started to review this important pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request include too many issues to be accepted as is. Please split each issue with the related code in separate pull requests, and resubmit. This will facilitate the revision and acceptation process, but also facilitate further support.
@@ -31,7 +36,8 @@ | |||
"PyYAML": "yaml", | |||
"pynrrd": "nrrd", | |||
"slicerio": "slicerio", | |||
"bids_validator": "bids_validator" | |||
"bids_validator": "bids_validator", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On top of this block, you should add a comment explaining why you define those variable on top of the initial method.
@@ -82,21 +89,42 @@ def check_and_install_python_packages(): | |||
IS_CLASSIFICATION_REQUESTED = True | |||
IS_SEGMENTATION_REQUESTED = True | |||
IS_SEMI_AUTOMATIC_PHE_TOOL_REQUESTED = True | |||
IS_MOUSE_SHORTCUTS_REQUESTED = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To which issue does it refer to?
KEYBOARD_SHORTCUTS_CONFIG_FILE_PATH = os.path.join(Path(__file__).parent.resolve(), "keyboard_shortcuts_config.yml") | ||
CLASSIFICATION_CONFIG_FILE_PATH = os.path.join(Path(__file__).parent.resolve(), "classification_config.yml") | ||
GENERAL_CONFIG_FILE_PATH = os.path.join(Path(__file__).parent.resolve(), "general_config.yml") | ||
REQUIRE_EMPTY = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please split the code according to each issue since too many issues within the same pull request complicates the revision and acceptation process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "split the code"? Kuan already knows that there are too many issues addressed here, and in the future we will make sure to submit smaller changes per PR, but for now, it will take more time to re-do multiple PRs, than to just review this PR and merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for further follow-up, it would be a great idea to track the code change for each issue (ex now, some block of code added in the pull request refer to undisclosed issue --- if in 6 months we discover a new bug related to a specific issue, I anticipate it will be very complicated to track which part of the code is associated with this issue when referring to this pull request that was supposed to address it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example of 'split the code', I mean ; download main branch --- create a branch for issue 40 --- add the code related to issue 40 --- submit a pull request --- Repeat for each issue addressed in this initial pull request. Total time estimated: 1 hour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kuan and I will meet at the end of this afternoon (16h45) to address it!
see #56 (comment) |
Fixes #53 : forced the window level and window width fields to only accept integers if CT has been selected in configuration
Fixes #49 : added a conditional catching if no file has been selected
Fixes #33: added a checkbox in the configuration window allowing to show/hide the LCD timer display during segmentation. Segmentation duration is still stored in
*SegmentationInformation.csv
. start/pause/restart buttons remainFixes #59: added a "Configure segmentation" window, replacing "Configure labels", where the labels, the semi_automatic-PHE_tool toggle and the timer display toggle are grouped
Fixes #57: paint, lasso paint, erase, segment editor, smooth margins, remove small holes now initiate the timer in the background
Fixes #60 : semi_automatic_phe_tool and timer both display correctly according to edited configurations
Fixes #40 : module can now be split in multiple files with the creation of
subFolder
withinsrc
. A better solution could be possible as the .py files in the subFolder cannot import the mainSlicerCART.py
, so variables must be redefined. If other .py files were placed in the same folder asSlicerCART.py
, Slicer treats them as scripted loadable modules which results in a bug upon startup of Slicer only.Fixes #37: user can now measure distances by using the
place a measurement line
button. all information on these markups will be saved inSegmentationInformation.csv
. Starting a measurement automatically starts the background timer too. Theundo
button now both remove segmentation and markup.--added
SELECTED_STYLE
as a global variable which has the valuesSlicer
,Dark Slicer
, orLight Slicer
to change the color of buttons in the UI for readability