Skip to content
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

Add RediMinds extension #1921

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add RediMinds extension #1921

wants to merge 2 commits into from

Conversation

kmordhwaj
Copy link

@kmordhwaj kmordhwaj commented Mar 15, 2023

New extension

  • 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.
  • 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.
  • Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (main, 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)
  • Hide unused features in the repository to reduce noise/irrelevant information:
    • Click Settings and in repository settings uncheck Wiki, Projects, and Discussions (if they are currently not used)
    • 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)

@pieper
Copy link
Member

pieper commented Mar 15, 2023

Interesting extension, thanks for making it available.

A few comments:

  • Be sure to add a License file and check the box on the checklist
  • I see on your website you use RediMinds but here you use Rediminds. I find the capital M helpful in parsing the name and suggest you use it here too.
  • This code looks fragile since there may be more than one segmentation in the scene. Maybe better to add a node combo box or use the currently selected SegmentEditor segmentation: https://github.com/Rediminds/SlicerRediminds/blob/main/Rediminds.py#L431-L432
  • I see you make a global python variable segtoken which is okay, but could be a name clash with some other module or code. Better to put that value in a namespaced spot, such as slicer.modules.rediminds.segtoken

@kmordhwaj
Copy link
Author

Hi @pieper
Thank you for making it correct.

I had added license and resolved the issues.

In my code everywhere I had replaced Rediminds with RediMinds.

I had made this accessible for the functionality when there is token. I had small check for getting actual segmentation node. I am comparing it with name. Yes, there is small loophole as the user(doctor) can rename the segmentation node then it will not send back to our server. But as I cant compare whole file properties as the main motto of making this extension is to do modification with segments (which will change the properties from that of initial node properties). It will not break any functionality.

I have removed the global variable segToken and placed it inside the class.

Thank you.

Also at last, I want to ask, I had renamed the SlicerRediminds.s4ext file to SlicerRediMinds.s4ext and "m" to "M" also inside this file. Is it okay to consider my pull request or I should make another one.

@pieper
Copy link
Member

pieper commented Mar 18, 2023

Your license file is not correct - it looks like you've mixed up Apache with another license.
Please check the text here. You can't change the text and call it the Apache license. https://www.apache.org/licenses/LICENSE-2.0

Thanks for changing to the capital M for RediMinds. That makes it easier to read. I think it would also help if you provided some more descriptive context for what RediMinds is so that people can understand why they might want to use the extension. From the website linked from the extension page I can't tell exactly what service or functionality you are providing. It's clear what the Slicer part does, but not what the backend does.

Just a hint for the future, here you could use slicer.util.getNode(RediMinds.nodename) to simplify the logic.

Also at last, I want to ask, I had renamed the SlicerRediminds.s4ext file to SlicerRediMinds.s4ext and "m" to "M" also inside this file. Is it okay to consider my pull request or I should make another one.

I don't see any reason you can't do that in this pull request.

@kmordhwaj
Copy link
Author

Thanks @pieper for the correction.

I made a correction to the License file. Also had written some more description for the purpose of this extension. I had also considered using slicer.util.getNode(RediMinds.nodename) to make it simpler.

Thank you.

@pieper
Copy link
Member

pieper commented Mar 18, 2023

Thanks for the quick updates 👍

I'll let others have a look and see if there are any other suggestions.

@lassoan
Copy link
Contributor

lassoan commented Mar 18, 2023

Category cannot be RediMinds (a category only makes sense if it is expected to contain a number of different extensions). I would recommend Informatics (the similar flywheel extension was added to this category) or Segmentation (if you RediMinds is mainly for segmentation).

I could not test the extension because the tutorial did not provide a demo account or any instructions on how to create an account for demo/evaluation/testing.

@kmordhwaj
Copy link
Author

Hi @lassoan sir.
Pardon me for the delay. I had placed our extension inside Segmentation category after discussion with client. Our client's having discussion on any other requirements in this extension and testing before publishing this extension. Also having discussion for providing a demo account credentials to you for testing.
Sorry.
Thanks.

@jcfr jcfr changed the title Merge Rediminds extension Add Rediminds extension Aug 15, 2023
@jcfr jcfr changed the title Add Rediminds extension Add RediMinds extension Aug 15, 2023
@jcfr
Copy link
Member

jcfr commented Aug 15, 2023

Also having discussion for providing a demo account credentials to you for testing.

Any progress on that front ?

@jcfr jcfr added the Status: Awaiting Response ⏳ Waiting for a response/more information label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting Response ⏳ Waiting for a response/more information
Development

Successfully merging this pull request may close these issues.

None yet

4 participants