code fixes issues #53, #49, #33, #59 - GUI update #56
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.
AcastaPaloma
changed the title
|
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! |
AcastaPaloma
changed the title
AcastaPaloma
changed the title
AcastaPaloma
changed the title
…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. |
| @@ -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.
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.
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.
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.
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.
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.
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.
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
subFolderwithinsrc. 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 linebutton. all information on these markups will be saved inSegmentationInformation.csv. Starting a measurement automatically starts the background timer too. Theundobutton now both remove segmentation and markup.--added
SELECTED_STYLEas a global variable which has the valuesSlicer,Dark Slicer, orLight Slicerto change the color of buttons in the UI for readability