ExtensionsIndex icon indicating copy to clipboard operation
ExtensionsIndex copied to clipboard

Add SlicerSPECTRecon extension

Open ObedDzik opened this issue 1 year ago • 1 comments

New extension

  • [x] Extension has a reasonable name (not too general, not too narrow, suggests what the extension is for)
  • [x] Repository name is Slicer+ExtensionName
  • [x] Repository is associated with 3d-slicer-extension GitHub topic so that it is listed here. To edit topics, click the settings icon in the right side of "About" section header and enter 3d-slicer-extension in "Topics" and click "Save changes". To learn more about topics, read https://help.github.com/en/articles/about-topics
  • [x] Extension description summarizes in 1-2 sentences what the extension is usable (should be understandable for non-experts)
  • [x] Any known related patents must be mentioned in the extension description.
  • [x] LICENSE.txt is present in the repository root. MIT (https://choosealicense.com/licenses/mit/) or Apache (https://choosealicense.com/licenses/apache-2.0/) license is recommended. If source code license is more restrictive for users than MIT, BSD, Apache, or 3D Slicer license then the name of the used license must be mentioned in the extension description.
  • [x] Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (main, release, ...) instead of a specific git hash to avoid re-submitting pull request whenever the extension is updated
  • [x] Extension icon URL is correct (do not use the icon's webpage but the raw data download URL that you get from the download button - it should look something like this: https://raw.githubusercontent.com/user/repo/main/SomeIcon.png)
  • [x] Screenshot URLs (screenshoturls) are correct, contains at least one
  • [x] Homepage URL points to valid webpage containing the following:
    • [x] Extension name
    • [x] Short description: 1-2 sentences, which summarizes what the extension is usable for
    • [x] At least one nice, informative image, that illustrates what the extension can do. It may be a screenshot.
    • [x] Description of contained modules: at one sentence for each module
    • [x] Tutorial: step-by-step description of at least the most typical use case, include a few screenshots, provide download links to sample input data set
    • [x] Publication: link to publication and/or to PubMed reference (if available)
    • [x] License: We suggest you use a permissive license that includes patent and contribution clauses. This will help protect developers and ensure the code remains freely available. We suggest you use the Slicer License or the Apache 2.0. Always mention in your README file the license you have chosen. If you choose a different license, explain why to the extension maintainers. Depending on the license we may not be able to host your work. Read here to learn more about licenses.
    • [x] Content of submitted json file is consistent with the top-level CMakeLists.txt file in the repository (dependencies, etc. are the same)
  • Hide unused features in the repository to reduce noise/irrelevant information:
    • [x] Click Settings and in repository settings uncheck Wiki, Projects, and Discussions (if they are currently not used)
    • [x] Click the settings icon next to About in the top-right corner of the repository main page and uncheck Releases and Packages (if they are currently not used)

ObedDzik avatar Aug 25 '24 06:08 ObedDzik

@lassoan let us know if there's anything you want us to add before merging ☺

lukepolson avatar Sep 06 '24 16:09 lukepolson

@lassoan hi team, we wanted to know if there is anything else to be done on our part for the successful merging of this extension.

ObedDzik avatar Nov 08 '24 02:11 ObedDzik

@lassoan @pieper

lukepolson avatar Nov 08 '24 19:11 lukepolson

Thank you for your excellent contribution and sorry for the long delay, we had a long backlog of extensions work through. The extension looks great, there is just one significant issue about how the required Python packages are installed, and a few minor tweaks:

  • [ ] Slicer startup after installing the extension appears to be hanging. Most users would force stop Slicer and conclude that the extension is broken. Actually, what's happening is that the extension installs Python packages at startup, which can take several minutes, and by the user aborting the installation the result is some incompletely installed Python packages, which actually do not work. All subsequent Slicer application starts are very slow, too (2-3x longer than usual) because the module spends a lot of time checking these packages and importing them. The solution is to not install any Python packages and refrain from importing any non-trivial packages in the main module .py at the file scope. Installing/importing packages inside functions - when actually needed - is OK. Installing/importing packages at the file scope in additional .py files (e.g., those in the Logic folder) is OK.
  • [ ] Fix project name and image URLs in CMakeLists.txt (see https://github.com/PyTomography/slicer_spect_recon/pull/36)
  • [ ] Change repository name to Slicer<ExtensionName>, which in this case is SlicerSPECTRecon
  • [ ] Rename the module name to SPECTRecon (we don't want each module name to start with Slicer), preferably change the module title to human-readable, such as SPECT reconstruction
  • [ ] Make link to the excellent user manual much more visible! I almost gave up on trying the extension when I accidentally noticed a little text mentioning User_Manual.md. It was not even a link that I could click. Make it big and bold and maybe show it at multiple places.
  • [ ] User interface section: mention the module name that should be opened, add a link to the user manual
  • [ ] It would be nice to have a very simple, basic non-video tutorial (just a few bullet points and screenshots). Having a video tutorial is great for some people, but many people will refuse to watch a 30-minute tutorial. Also, you can ensure that a text-based tutorial is always up-to-date, but it is practically impossible to update video tutorials up-to-date, so they will always be somewhat inaccurate.

lassoan avatar Dec 11 '24 22:12 lassoan

Hi @lassoan ,

Thanks for the kind words about the extension and sorry for the late reply. I believe I have now addressed all the issues you raised above in the main repository https://github.com/PyTomography/Slicer/tree/main

I just need @ObedDzik to change the commit from this link to match the new title. Obed I made a pull request here, Andras can you also confirm that these are the required changes based on what you mentioned above?

Also @lassoan , I've changed the repository name here to be "Slicer". Is that what you meant?

lukepolson avatar Jan 07 '25 01:01 lukepolson

Thank you, I'll review the updates tomorrow. To just quickly respond to the repository and extension name: The repository name should start with Slicer followed by the extension's name. Sorry, I missed the "start" word in my comment. So, please rename the repository from Slicer to SlicerSPECTRecon or SlicerSPECTReconstruction.

lassoan avatar Jan 07 '25 03:01 lassoan

Thanks @lassoan , and don't hesitate to ask if there's anything else that would help improve the module : ). It's been a real pleasure working/learning with slicer

lukepolson avatar Jan 10 '25 06:01 lukepolson

Hi @lassoan , any updates on this?

lukepolson avatar Jan 20 '25 17:01 lukepolson

Hi @jamesobutler @lassoan , anything else we can do?

lukepolson avatar Jan 27 '25 19:01 lukepolson

@lassoan any advice for what went wrong with the checks? Apologies for for messaging so much, but there's a couple groups interested in using the extension for some of their own recons : )

lukepolson avatar Jan 29 '25 22:01 lukepolson

@lassoan @jamesobutler please let us know the next steps

lukepolson avatar Feb 06 '25 04:02 lukepolson

Thanks for your patience. When installing the extension into a fresh Slicer installation (that does not have pytomography installed) I get this error:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:\D\S4R\python-install\Lib\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 "C:/D/S4R/Slicer-build/slicer.org/Extensions-33204/SPECTRecon/lib/Slicer-5.7/qt-scripted-modules/SPECTRecon.py", line 49, in <module>
    from Logic.SPECTReconLogic import SPECTReconLogic
  File "C:\D\S4R\Slicer-build\slicer.org\Extensions-33204\SPECTRecon\lib\Slicer-5.7\qt-scripted-modules\Logic\SPECTReconLogic.py", line 8, in <module>
    from pytomography.io.SPECT import dicom
ModuleNotFoundError: No module named 'pytomography'
[Qt] loadSourceAsModule - Failed to load file "C:/D/S4R/Slicer-build/slicer.org/Extensions-33204/SPECTRecon/lib/Slicer-5.7/qt-scripted-modules/SPECTRecon.py"  as module "SPECTRecon" !
[Qt] Fail to instantiate module  "SPECTRecon"
[Qt] The following modules failed to be instantiated:
[Qt]    SPECTRecon

You can fix this by making sure you don't import anything in the main scope of the module (e.g., move from Logic.SPECTReconLogic import SPECTReconLogic into the functions where you actually need to use SPECTReconLogic.

lassoan avatar Feb 06 '25 10:02 lassoan

@lassoan I just pushed an update to the main repo, do the tests need to rerun?

lukepolson avatar Feb 10 '25 01:02 lukepolson

@lassoan @jamesobutler any updates? Is it possible to run these checks again?

lukepolson avatar Mar 13 '25 15:03 lukepolson

Thanks for your patience. I've tested installation and running the module (just basic smoke test) and found these:

  1. After installing the extension and switching to the module I get this error (and a blank module widget):
Traceback (most recent call last):
  File "C:/D/SlicerSPECTRecon-win64rel/_CPack_Packages/win64/ZIP/33120-win-amd64-SPECTRecon-gitb34c85c-2025-02-09/lib/Slicer-5.7/qt-scripted-modules/SPECTRecon.py", line 140, in setup
    from Logic.SPECTReconLogic import SPECTReconLogic
  File "C:\D\SlicerSPECTRecon-win64rel\_CPack_Packages\win64\ZIP\33120-win-amd64-SPECTRecon-gitb34c85c-2025-02-09\lib\Slicer-5.7\qt-scripted-modules\Logic\SPECTReconLogic.py", line 26, in <module>
    from Logic.AdvancedPSF import *
ModuleNotFoundError: No module named 'Logic.AdvancedPSF'
Traceback (most recent call last):
  File "C:/D/SlicerSPECTRecon-win64rel/_CPack_Packages/win64/ZIP/33120-win-amd64-SPECTRecon-gitb34c85c-2025-02-09/lib/Slicer-5.7/qt-scripted-modules/SPECTRecon.py", line 240, in enter
    self.initializeParameterNode()
  File "C:/D/SlicerSPECTRecon-win64rel/_CPack_Packages/win64/ZIP/33120-win-amd64-SPECTRecon-gitb34c85c-2025-02-09/lib/Slicer-5.7/qt-scripted-modules/SPECTRecon.py", line 294, in initializeParameterNode
    self.setParameterNode(self.logic.getParameterNode())
AttributeError: 'NoneType' object has no attribute 'getParameterNode'

This is caused by AdvancedPSF.py not being included in the package (need to update the CMakeLists.txt file).

    1. On Windows, there is a long (15-20 second) delay at every time you open the module, installing required dependencies. Since pip's requirement checks are really slow, most extension do quick tests if specific Python packages are installed and only ask pip to install if something is missing.

Without fixing 1) users would not be able to use the extension, so that should be fixed before the extension appears in the Extensions Manager. It is just adding a single line to the CMakeLists.txt file. Let me know when it's done and then this pull request should be ready to merged.

It is up to you if you want to address 2) now or later. Once the extension is added to the ExtensionsIndex, it gets updated every night, so any changes that you make automatically become available for users the next day.

lassoan avatar Mar 13 '25 16:03 lassoan

@lassoan just made the changes. Let me know if theres any issues.

lukepolson avatar Mar 13 '25 17:03 lukepolson

The extension should appear tomorrow for the Slicer Preview Release that you download tomorrow. If everything works well then submit a pull request that adds this .json file to the 5.8 branch so that users get this extension for the latest Slicer Stable Release as well.

lassoan avatar Mar 13 '25 20:03 lassoan

@jamesobutler @lassoan I've made some changes to the exntesion but they don't seem to appear in the current Slicer 5.9 on the extensions index in the UI. For example, see the commits here

https://github.com/PyTomography/SlicerSPECTRecon/commits/main/

but the "last update" according to the extensions manager is Match 14. As I understood, the extensions index gets updated every night. Is there something else I need to do to ensure the changes get made in the extensions index?

lukepolson avatar Apr 03 '25 04:04 lukepolson

Maybe you expected to see updated extensions for a Slicer Preview Release? This would not be practically feasible, because there are hundreds Slicer Preview Release versions (a new one is created every night). See how to get updated extensions here:

  • https://slicer.readthedocs.io/en/latest/user_guide/extensions_manager.html#update-extensions
  • https://slicer.readthedocs.io/en/latest/developer_guide/extensions.html#how-often-extensions-are-uploaded-on-the-extensions-server

lassoan avatar Apr 03 '25 04:04 lassoan