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

⚗️✨♻️ Support bundled transforms #36

Merged
merged 15 commits into from
Jun 24, 2022
Merged

Conversation

dyollb
Copy link
Owner

@dyollb dyollb commented Jun 22, 2022

What do these changes do?

This PR implements

  • loading pre-processing transforms from the config file (using monai.bundle)
  • implements loading yml (and json) config files
  • adds tests to check/understand behavior of bundle.ConfigParser
  • removes save_nifti branch during training, i.e. automatic predicting based on validation data
  • default pre-processing/augmentation now moves data to GPU after CropForegroundd

Related issue/s

How to test

Checklist

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • Unit tests for the changes and run locally with the command pytest tests
  • Documentation reflects the changes

@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #36 (0a67c03) into main (5d0b862) will increase coverage by 1.16%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
+ Coverage   17.32%   18.48%   +1.16%     
==========================================
  Files          44       45       +1     
  Lines        2211     2229      +18     
  Branches      365      366       +1     
==========================================
+ Hits          383      412      +29     
+ Misses       1805     1794      -11     
  Partials       23       23              
Flag Coverage Δ
unittests 18.48% <50.00%> (+1.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/segmantic/util/json.py 71.42% <ø> (ø)
src/segmantic/seg/monai_unet.py 23.58% <14.81%> (+1.69%) ⬆️
src/segmantic/commands/monai_unet_cli.py 67.56% <66.66%> (+9.67%) ⬆️
src/segmantic/prepro/labels.py 88.88% <88.88%> (+0.17%) ⬆️
src/segmantic/__init__.py 100.00% <100.00%> (ø)
src/segmantic/seg/dataset.py 83.87% <100.00%> (ø)
src/segmantic/util/cli.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d0b862...0a67c03. Read the comment docs.

@dyollb dyollb self-assigned this Jun 24, 2022
@dyollb dyollb requested a review from pcrespov June 24, 2022 08:25
Copy link
Collaborator

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

  • Interesting the validation of path
  • I did not know about the warnings with codecov 👍 . You will achieve 100% coverage in no time :-)

scripts/check_masks.py Outdated Show resolved Hide resolved
src/segmantic/commands/monai_unet_cli.py Show resolved Hide resolved
src/segmantic/util/cli.py Outdated Show resolved Hide resolved


def test_validate_args():
args1 = {"file_path": "/path/file.txt", "arg_int": 10, "arg_float": 5.0}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!
Why dont' you parametrize these?
When you have to test more variants it will be difficult to check whichone failed

https://docs.pytest.org/en/6.2.x/example/parametrize.html#paramexamples

Something like (please double check) ...

params_data = {
 "args1_skip_default": ({"file_path": "/path/file.txt", "arg_int": 10}, False), 
  "args1_wrong_type": ({"file_path": 23, "arg_int": 10, "arg_float": 5}, True)
}

@pytest.mark.parametrize(
   "arguments,expected_failure", list(params_data.values()), ids=list(params_data.keys())
def test_validate_args(arguments, expected_failure):
   ...

Copy link
Owner Author

Choose a reason for hiding this comment

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

nice!

@dyollb
Copy link
Owner Author

dyollb commented Jun 24, 2022

Thanks @pcrespov for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants