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 square, cube and threshold to cli #137

Merged
merged 31 commits into from
Dec 3, 2020
Merged

Add square, cube and threshold to cli #137

merged 31 commits into from
Dec 3, 2020

Conversation

maheuss
Copy link
Contributor

@maheuss maheuss commented Oct 8, 2020

Create CLI for creating mask

Implementation of st_mask_cube, st_mask_threshold and st_mask_square which allow to apply these masks to the desired files. Each function has its own required input sizes.

Fixes #88
Related to #66

shimmingtoolbox/cli/st_mask_square.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/st_mask_square.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/st_mask_square.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/st_mask_cube.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Member

@po09i po09i left a comment

Choose a reason for hiding this comment

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

Just a couple of suggestions to make things more uniform.

  • Name of CLI should not have shimmingtoolbox in the name. --> st_mask
  • The main could be the same name as the file (mask).

Output: The CLI should save the mask as a nifti array using an option spcifying where to put the output file. The fullpath should be returned.

setup.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/mask_shimmingtoolbox.py Outdated Show resolved Hide resolved
test/cli/test_cli_mask_shimmingtoolbox.py Outdated Show resolved Hide resolved
@maheuss maheuss marked this pull request as ready for review November 6, 2020 03:33
@po09i po09i requested a review from jcohenadad November 6, 2020 21:04
@po09i
Copy link
Member

po09i commented Nov 6, 2020

LGTM!

Copy link
Contributor

@jidicula jidicula left a comment

Choose a reason for hiding this comment

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

Looks good! Just one small change requested so we can keep our docs up-to-date with the library code:

Could you add this CLI to the documentation? An example can be seen here for download_data. If you want to check that it builds correctly with Sphinx, you can cd to docs/ and run make html, then check shimming-toolbox/docs/build/html/6_api_reference/api.html in any web browser. If you're not able to build the docs locally, you can push your .rst changes anyway and check the branch preview on ReadTheDocs:
Screen Shot 2020-11-08 at 14 27 05

shimmingtoolbox/cli/mask.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/mask.py Outdated Show resolved Hide resolved
docs/source/6_api_reference/api.rst Outdated Show resolved Hide resolved
shimmingtoolbox/cli/mask.py Outdated Show resolved Hide resolved
@po09i
Copy link
Member

po09i commented Dec 3, 2020

Remaining work to do summarized in this issue. This can be merged.

@jidicula jidicula self-requested a review December 3, 2020 19:38
Copy link
Contributor

@jidicula jidicula left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@po09i po09i added this to In progress in Get a CLI based working example script via automation Dec 3, 2020
@po09i po09i added the feature Introduces a new functionality label Dec 3, 2020
Get a CLI based working example script automation moved this from In progress to Reviewer approved Dec 3, 2020
@po09i po09i merged commit 2d84845 into master Dec 3, 2020
Get a CLI based working example script automation moved this from Reviewer approved to Done Dec 3, 2020
@po09i po09i deleted the mh/88_st_mask branch December 3, 2020 20:05
kousu pushed a commit that referenced this pull request Mar 20, 2021
* First attempt for st_mask w/ some questions

* Updated documentation & docstrings

* Implementation of CLI allowing to use the following masking techniques: square, cube and threshold

* Removal of st_mask.py

* Update setup.py

Added masking cli in entry_points and SCT library

* Changing function name and checking dimensions

* Changing function name and checking 2 dimensions

* Changing function name

* Grouping of 3 masks & addition of functionnalities

* Added command mask_shimmingtoolbox

* Implementing the test for mask_shimmingtoolbox

* Added output nifti file where the mask is stored

* Adaptation of input and output file for the test

* Modification of entry points

* Small comment fixes

* Simplified implementation of sizes and cen_dim

* Adaptation of tests according to size

* Addition of mask in the documentation

* addition of the extension sphinx_click.ext

* Use of click instead of automodule

* Addition of a cli tab in the documentation

* Addition of the sphix-click library

* Changing the name of the function for mask_cli

* Improvement of descriptors and change for box/rect

* Adaptation for box and rect masks

* Fixed documentation issue

* Added mask_cli in the documentation

* Change output to file instead of folder

* Adaptation of the temporary output of the test

Co-authored-by: Alexandre D'Astous <po09i@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Introduces a new functionality
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Create CLI for creating mask
4 participants