Skip to content

Conversation

@che85
Copy link
Contributor

@che85 che85 commented Apr 25, 2022

Thank you for contributing to 3D Slicer!

  • To add a new extension with this pull request: Please keep content of "TODO list for submitting a new extension" section and put an 'x' character in the brackets for each todo item to indicate that you have accomplished that prerequisite.

  • To update an existing extension with this pull request: Please delete all text in this template and just describe which extension is updated and optionally tell us in a sentence what has been changed. To make extension updates easier in the future (so that you don't have to submit a new pull request after each change in your extension), you may consider replacing specific git hash in your s4ext file by a branch name (for example: master or release for nightly version; (majorVersion).(minorVersion) such as 4.10 for stable Slicer version).

TODO list for submitting a new extension

[To make sure users can find your extension, understand what it is intended for and how to use it, please complete the checklist below. You do not need to complete all the item by the time you submit the pull request, but most likely the changes will only be merged if all the tasks are done. See more information about the submission process here: https://slicer.readthedocs.io/en/latest/developer_guide/extensions.html]

  • Extension has a reasonable name (not too general, not too narrow, suggests what the extension is for)
  • Repository name is Slicer+ExtensionName
  • 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
  • Extension description summarizes in 1-2 sentences what the extension is usable (should be understandable for non-experts)
  • Any known related patents must be mentioned in the extension description.
  • If source code license is more restrictive for users than BSD, MIT, Apache, or 3D Slicer license then the name of the used license must be mentioned in the extension description.
  • Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (main, master, release, ...) instead of a specific git has to avoid re-submitting pull request whenever the extension is updated
  • 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)
  • Screenshot URLs (screenshoturls) are correct, contains at least one
  • Homepage URL points to valid webpage containing the following:
    • Extension name
    • Short description: 1-2 sentences, which summarizes what the extension is usable for
    • At least one nice, informative image, that illustrates what the extension can do. It may be a screenshot.
    • Description of contained modules: at one sentence for each module
    • 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
    • Publication: link to publication and/or to PubMed reference (if available)
    • 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.
    • Content of submitted s4ext file is consistent with the top-level CMakeLists.txt file in the repository (description, URLs, dependencies, etc. are the same)

[If you have any questions or comments then please describe them here.]

@pieper
Copy link
Member

pieper commented Apr 25, 2022

Can this have a more descriptive name? I see you don't have much documentation yet, so I'm not sure what a good name would be, but maybe it's related to skeletonizing segmentations? It's not clear what CMRep is.

@che85
Copy link
Contributor Author

che85 commented Apr 25, 2022

More information can be found on https://cmrep.readthedocs.io/en/latest/about.html

I can add the link to the description.

@che85
Copy link
Contributor Author

che85 commented Apr 25, 2022

@pieper Let me know if there is anything else? I just want to get this rolling so I can ask out collaborators to test and give feedback.

@pieper
Copy link
Member

pieper commented Apr 25, 2022

CMRep is still a bit obscure to me, but if you think it's a well-established term in the field it might be okay but I would vote for something more descriptive like SlicerMedialRep so there's a least a hint.

@pieper Let me know if there is anything else?

Are you planning to do the rest of the items on the checklist?

@jamesobutler
Copy link
Contributor

Reviewing https://cmrep.readthedocs.io/en/latest/building.html#required-packages, it says that ITK 4.13.2 and VTK 6.3.0 are required. Slicer master and latest Slicer stable 4.11.20210226 both require versions of ITK and VTK that are newer than that. Is your documentation out-of-date? Does your extension build with latest Slicer successfully?

@che85
Copy link
Contributor Author

che85 commented Apr 25, 2022

@jamesobutler We are only using a few header files as of now from the original cmrep repository (https://github.com/pyushkevich/cmrep).

The SlicerCMRep extension builds fine on my current version of Slicer.

@lassoan
Copy link
Contributor

lassoan commented Apr 25, 2022

I agree that for most users CMRep does not mean anything. Therefore, most likely users would not discover this module by themselves. This is somewhat a missed marketing opportunity, but may make sense if advertising the CMRep "brand" is preferable. It is also fine to have an obscure extension name for an infrastructural extension, which is mainly used by other extensions. So, overall I think the SlicerCMRep name is OK - similarly to SlicerElastix, SlicerVMTK, SlicerANTs.

@che85
Copy link
Contributor Author

che85 commented Apr 25, 2022

@pieper The unchecked items are still WIP but planning on adding tutorial, icon, etc once successfully tested.

@jamesobutler
Copy link
Contributor

In regards to the License checkpoint - this is going to be GPL-3.0 like the upstream (https://github.com/pyushkevich/cmrep)?

@pieper
Copy link
Member

pieper commented Apr 25, 2022

@pieper The unchecked items are still WIP but planning on adding tutorial, icon, etc once successfully tested.

If merging this now will facilitate testing and tutorial development I have no problem with that.

@lassoan
Copy link
Contributor

lassoan commented Apr 25, 2022

In regards to the License checkpoint - this is going to be GPL-3.0 like the upstream (https://github.com/pyushkevich/cmrep)?

This is a good point, we need to add a license file to the repository.

For the code that we write we should be able to use a permissive license (BSD, MIT, or Apache). For the code that is based on what others developed before, we would need to ask Alison if we can distribute with a permissive license.

Ideally, we would agree to use a simple permissive license for the whole repository, but if that is not feasible then we could perhaps add a license file that describes what license applies to each folder.

We also need to add to the extension description which modules use code with copyleft license.

@lassoan
Copy link
Contributor

lassoan commented Apr 25, 2022

  • Any known related patents must be mentioned in the extension description.

We need to ask Paul Yushkevich if they submitted any patent applications for cmrep. It seems that Paul has two patents, but I'm not sure if they cover any part of cmrep, or the methods described just use cmrep.

cmrep is distributed with GPL license, which grants royalty-free patent usage rights, so maybe that's sufficiently answers patent usage concerns.

@pieper
Copy link
Member

pieper commented Apr 25, 2022

Agreed, it would be much nicer if Alison and Paul could accept a more permissive license for the files used here.

@che85 che85 changed the title ENH: adding SlicerCMRep extension ENH: adding MedialSkeleton extension Apr 27, 2022
@che85
Copy link
Contributor Author

che85 commented Apr 27, 2022

Everyone, the extension was renamed to "MedialSkeleton" and we received permission to distribute the required files (in the red boxes of the diagram) from the cm-rep repository under the Slicer BSD license. The resulting extension does not depend on cm-rep anymore so we don't have any license conflicts anymore.

overview

@che85
Copy link
Contributor Author

che85 commented Apr 27, 2022

Let me know if anything else is needed. Otherwise, it would be great if we could get this into the nightly asap. Thank you.

@pieper
Copy link
Member

pieper commented Apr 28, 2022

Looks good @che85 . I'll merge now and we can wrap up anything else after you do some testing.

@pieper
Copy link
Member

pieper commented Apr 28, 2022

Note the commit check failing, so changing the repo name to SlicerMedialSkeleton is suggested.

@pieper pieper merged commit c1becd0 into Slicer:master Apr 28, 2022
@lassoan
Copy link
Contributor

lassoan commented Apr 28, 2022

Yes, the extension name is good (MedialSkeleton) but for extension name we should add the Slicer prefix. Let's keep everything as is for now to see if the build works well, and then tomorrow we can update the repository name and s4ext file.

@jamesobutler
Copy link
Contributor

jamesobutler commented Apr 28, 2022

Is https://github.com/JolleyLab/MedialSkeleton also valid as a fork of https://github.com/pyushkevich/cmrep_skelGUI? It appears like it should be its own independent repository since it doesn’t seem to match what is seen in the upstream.

@jcfr
Copy link
Member

jcfr commented Apr 29, 2022

@che85 As hinted by @jamesobutler , would it be possible to perform the following:

Create an independent project:

Looks like the culprit is the folder build/. This could be achieved leveraging https://github.com/jcfr/dockgit-rocket-filter.

$ git_list_largest_file_from_history.sh --human-readable --table 15
size pack_size sha location
68MiB 14MiB 3fe4bcb30655142fad42a1f3fc363fae7ed5a665 build/ipch/eventqtslotconnect-f143a569/eventqtslotconnect-4aece855.ipch
53MiB 16MiB f4d951a5496608dd79bb32231ab7054ccb1cb9a7 build/EventQtSlotConnect.sdf
52MiB 11MiB c1100c62ec28c67d4a82a73ec49e9819ee87a211 build/ipch/eventqtslotconnect-f143a569/eventqtslotconnect-a51ca59b.ipch
28MiB 5.9MiB e8315b7f7a185389a819d0c67adb8580b6601560 build/Debug/eventqtslotconnect.pdb
17MiB 3.6MiB 5933dff9ed60712c9c33521bd4f9d92bc32c557a build/Release/EventQtSlotConnect.ilk
16MiB 3.0MiB 1cd4a9120d5ed6932b3bc38f4ccd1b3eb1eb5cbe build/Debug/EventQtSlotConnect.ilk
14MiB 3.1MiB ba10c3e72cdb09d4f8f799e9f0f46ff4263baf4d build/Release/eventqtslotconnect.pdb
14MiB 3.1MiB f1fb67ea52f15baaa50fd853b201b83e2377d740 build/Debug/EventQtSlotConnect.exe
7.4MiB 2.2MiB 3ed1c83e7421b8f9a2ff3a14d4c72440d9173c8f build/Release/EventQtSlotConnect.exe
2.4MiB 672KiB 46a26cafc403ffb325a15b6e340b81842a5525dd build/Debug/testhatskel.vtk
2.3MiB 638KiB 1ec4c12cb1881790b8e56a90d03910091c11f804 build/Debug/mask_boundary_Skel.vtk
2.2MiB 631KiB 8445af50af1775e0da04d865ceb58cd4d7bdd8d4 build/Debug/mask_boundary_skeleton.vtk
858KiB 456KiB da53d265affda4b839d54ff55db68c539e1bde9f build/Debug/testhatsmooth2.vtk
825KiB 257KiB 8ab19619413f5f46b886a822536b4555371140ef build/Debug/mask_boundary.vtk
690KiB 644KiB 9cc3c809d5eb91058865d94acc90d29914d3f3a0 SkeletonTool/Screenshots/SkeletonTool01.png

@jcfr
Copy link
Member

jcfr commented Apr 29, 2022

Here is a cleanup project: https://github.com/jcfr/MedialSkeleton

The following command was used:

# Download helper script
curl https://raw.githubusercontent.com/jcfr/dockgit-rocket-filter/master/git-rocket-filter.sh \
  -o ~/bin/git-rocket-filter && chmod +x ~/bin/git-rocket-filter

# Download current project
git clone JolleyLab/MedialSkeleton

cd MedialSkeleton

# Filter
git-rocket-filter -b newMaster --remove /build --remove "/*~" --force

# Create new project
git remote add jcfr git@github.com:jcfr/MedialSkeleton.git

# Publish
git push jcfr newMaster:main

comparison

$ time git clone jcfr/MedialSkeleton MedialSkeleton MedialSkeleton-jcfr
Cloning into 'MedialSkeleton-jcfr'...
[...]
Receiving objects: 100% (410/410), 3.02 MiB | 3.38 MiB/s, done.
Resolving deltas: 100% (233/233), done.

real	0m2.195s
user	0m0.406s
sys	0m0.163s

$ time git clone JolleyLab/MedialSkeleton MedialSkeleton-JolleyLab
Cloning into 'MedialSkeleton-JolleyLab'...
[...]
Receiving objects: 100% (602/602), 66.27 MiB | 3.89 MiB/s, done.
Resolving deltas: 100% (321/321), done.

real	0m18.773s
user	0m6.580s
sys	0m1.499s

image

@che85
Copy link
Contributor Author

che85 commented Apr 29, 2022

Done. @jcfr I basically just recreated a new local repository and pushed your copy to ours. Hope that works. Thank you

@jcfr
Copy link
Member

jcfr commented Apr 29, 2022

Make sure to set the new repo to public, it currently is not available at https://github.com/JolleyLab/MedialSkeleton

@che85
Copy link
Contributor Author

che85 commented Apr 29, 2022

Good point. Done

@jcfr
Copy link
Member

jcfr commented Apr 29, 2022

Thanks @che85

Few additional nitpicks:

Reduce noise updating setting

  • hide Project & wiki tabs
  • hide Packages and Releases

image

Set description

image

@lassoan
Copy link
Contributor

lassoan commented Apr 29, 2022

I've made Project & wiki tabs, Packages and Releases hidden. I'll now rename the repository to SlicerMedialSkeleton (keeping the extension name MedialSkeleton) and update the s4ext file accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants