Skip to content

README.md Update and Documentation Proposal #105

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

Closed
wants to merge 18 commits into from

Conversation

maxradx
Copy link
Member

@maxradx maxradx commented Nov 18, 2024

Hi Team,

This Pull Request updates the README.md and adds essential Documentation for testing, from which further comments/documentation can be built on.

How to test this PR? (3 steps):

1- Start from scratch, do as it is the first time you use slicer. Follow Installation steps from the README.
2- Try QuickSTART in the README.md.
3- Read Documentation from Documentation Welcome Page.

Question to ask yourself (examples):

A. Was the installation process without obstruction and difficulties (Slicer and SlicerCART)?
B. Was I able to load images from my dataset? Once loaded, was I able to do manual segmentation as written in the QuickStart?
C. Any feedback/comments regarding the organization set-up of the documentation? When you read those instructions, are they easy for you to understand?

Important Note.
This PR is a pre-beta-release version of SlicerCART. That means the actual main script is really NOT READY for deployment. Only one basic scenario is intended to be tested for now (viewing images and annotate them). Essential functionalities remain to be implemented (bugs fixing e.g. changing labels, load previous segmentations, compare segments, go automatically to the next case, etc.). Accordingly, you should approach this PR as a Documentation PR as mentioned above: are the steps for begin using SlicerCART clear? (Your feedback will be wanted in future PR related to specific functionalities)

This PR addresses issue #62.

Thank you for your consideration.

Maxime

This was unlinked from issues Nov 18, 2024
This was referenced Nov 18, 2024
@maxradx maxradx added documentation Improvements or additions to documentation help wanted Extra attention is needed priority high To manage as soon as possible. Anything within the big picture, not nit-picky. labels Nov 18, 2024
Copy link
Member

@sandrinebedard sandrinebedard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting all of this together! I followed the instructions, I left some minor comments about the instructions, in general, it is okay!

However, I cannot load the module (step 7):

Capture d’écran, le 2024-11-19 à 16 46 19

when I search for it, it is says it can't be loaded:
Capture d’écran, le 2024-11-19 à 16 47 01

@maxradx
Copy link
Member Author

maxradx commented Nov 20, 2024

Thank you for putting all of this together! I followed the instructions, I left some minor comments about the instructions, in general, it is okay!

However, I cannot load the module (step 7):

Capture d’écran, le 2024-11-19 à 16 46 19

when I search for it, it is says it can't be loaded: Capture d’écran, le 2024-11-19 à 16 47 01

Ok! 1- Now with the new instructions adapted, can you try it again? (maybe that is because of the part of the comment you left where you did not know what I meant! --- ex drag the SlicerCART.py file in the additional module path section (see step 4 in the most recent committed version --- need to fetch and pull)

2- Can you open the python console and screenshot the line where there is the error? (there should have some errors when a slicer module cannot be instantiated).

Thanks for your feedback: greatly appreciated!

@sandrinebedard
Copy link
Member

2- Can you open the python console and screenshot the line where there is the error? (there should have some errors when a slicer module cannot be instantiated).

same thing again, here is the error in the python console:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Applications/Slicer.app/Contents/lib/Python/lib/python3.9/imp.py", line 169, in load_source
    module = _exec(spec, sys.modules[name])
  File "<frozen importlib._bootstrap>", line 613, in _exec
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/SlicerCART.py", line 10, in <module>
    from utils import * # Import all modules, packages and global variables
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/__init__.py", line 1, in <module>
    from .global_variables import *
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/global_variables.py", line 3, in <module>
    from utils.requirements import *
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/requirements.py", line 27, in <module>
    check_and_install_python_packages()
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/install_python_packages.py", line 31, in check_and_install_python_packages
    response = qt.QMessageBox.question(slicer.util.mainWindow(),
NameError: name 'qt' is not defined
[Qt] loadSourceAsModule - Failed to load file "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/SlicerCART.py"  as module "SlicerCART" !
[Qt] Fail to instantiate module  "SlicerCART"
[Qt] The following modules failed to be instantiated:
[Qt]    SlicerCART
[Qt] Populating font family aliases took 138 ms. Replace uses of missing font family ".AppleSystemUIFont" with one that exists to avoid this cost. 

![](images/module_filepath.png)

(N.B. 1- You must have the file:
if it is the folder path, then the module will not work; 2- in the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is the folder path

if what is the folder path? this sentence is unclear, I get that the SlicerCART.py needs to be in the folder, but this is not what your sentence is telling me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I see. Adapted. Better?

@maxradx
Copy link
Member Author

maxradx commented Nov 20, 2024

2- Can you open the python console and screenshot the line where there is the error? (there should have some errors when a slicer module cannot be instantiated).

same thing again, here is the error in the python console:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Applications/Slicer.app/Contents/lib/Python/lib/python3.9/imp.py", line 169, in load_source
    module = _exec(spec, sys.modules[name])
  File "<frozen importlib._bootstrap>", line 613, in _exec
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/SlicerCART.py", line 10, in <module>
    from utils import * # Import all modules, packages and global variables
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/__init__.py", line 1, in <module>
    from .global_variables import *
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/global_variables.py", line 3, in <module>
    from utils.requirements import *
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/requirements.py", line 27, in <module>
    check_and_install_python_packages()
  File "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/utils/install_python_packages.py", line 31, in check_and_install_python_packages
    response = qt.QMessageBox.question(slicer.util.mainWindow(),
NameError: name 'qt' is not defined
[Qt] loadSourceAsModule - Failed to load file "/Users/sandrinebedard/code/slicer-manual-annotation/SlicerCART/src/SlicerCART.py"  as module "SlicerCART" !
[Qt] Fail to instantiate module  "SlicerCART"
[Qt] The following modules failed to be instantiated:
[Qt]    SlicerCART
[Qt] Populating font family aliases took 138 ms. Replace uses of missing font family ".AppleSystemUIFont" with one that exists to avoid this cost. 

Ok! Thanks for the input.
If you copy paste this command in the slicer python console, and then try to use the module, what does it says? (success or error in python console)

import slicer
from slicer.util import qt

thanks!

@jcohenadad jcohenadad self-requested a review November 20, 2024 16:58
@jcohenadad
Copy link
Member

@maxradx a lot of suggested commits are a month-old. Can you please accept them so we can make additional commit suggestions?

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of additional images in this PR. Although the intention is good (screenshots for the documentation to improve user experience), adding binary files to a git repository is problematic. Given that these screenshots belong to a specific version of slicerCART, which is bound to evolve with time, all these screenshots will need to be updated, again and again, with the repository accumulating fat overtime. In general, it is not a good practice to upload binaries to a git repository. See eg:

I suggest opening another repos to store the images, eg: slicer-cart-media.

Also important: make sure to provide the sources of your edited screenshots. Eg: what if you made a typo in the edited screenshot, that someone would like to fix later? Working with cloud solution (eg drawIO, google slide) makes it easy to modify images.

Finally: because of the above issues, I think this PR should not be merged as is with all these binaries. And if they are removed from the branch (following my recommendations), then that branch, if accepted, should absolutely be 'squashed' before merging, otherwise the binaries will exist in the git history and it will be a pain to remove them.

maxradx added a commit that referenced this pull request Jun 1, 2025
Updated README.md and the documentation from issue 62

In that context, the PR #105 will be closed.
@maxradx
Copy link
Member Author

maxradx commented Jun 1, 2025

@jcohen I agree with your general discussion. Comments addressed, but the overall updated documentation will be soon completed in future PRs, considering advancements in SlicerCART. Some reviews from this PR have been translated in PR #188. In that context (repo also renamed), this PR is closed, and future work will address completely its recommendation (currently: partial).

@maxradx maxradx closed this Jun 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation help wanted Extra attention is needed priority high To manage as soon as possible. Anything within the big picture, not nit-picky.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve README
4 participants