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

✨added cross-validation capabilities #46

Merged
merged 23 commits into from
Jul 23, 2022

Conversation

Konohana0608
Copy link
Collaborator

What do these changes do?

The changes should enable the user to run a cross validation. At the moment it is with the old implementation style using
.json files. Will need to adapt the code to work with the newer .yamal files instead.

Related issue/s

How to test

Checklist

@Konohana0608 Konohana0608 requested a review from dyollb July 4, 2022 14:25
@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #46 (37c9799) into main (61a3762) will increase coverage by 0.82%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   19.48%   20.30%   +0.82%     
==========================================
  Files          47       47              
  Lines        2392     2531     +139     
  Branches      395      419      +24     
==========================================
+ Hits          466      514      +48     
- Misses       1900     1990      +90     
- Partials       26       27       +1     
Flag Coverage Δ
unittests 20.30% <41.66%> (+0.82%) ⬆️

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

Impacted Files Coverage Δ
src/segmantic/commands/monai_unet_cli.py 50.98% <12.50%> (-17.60%) ⬇️
src/segmantic/seg/monai_unet.py 22.08% <13.33%> (-3.65%) ⬇️
src/segmantic/seg/dataset.py 88.33% <90.32%> (+2.96%) ⬆️

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 61a3762...37c9799. Read the comment docs.

src/segmantic/seg/dataset.py Outdated Show resolved Hide resolved
src/segmantic/seg/dataset.py Outdated Show resolved Hide resolved
src/segmantic/seg/dataset.py Outdated Show resolved Hide resolved
@dyollb
Copy link
Owner

dyollb commented Jul 6, 2022

why don't you write a little test in tests/seg/test_dataset.py to make sure the dataset kfold splitting is doing what you expect?

Ps. the magic variable tmp_path: Path e.g. here is a temporary directory path, where you can create files and they will be cleaned up after the test has run.
Ps2. this will also solve your coverage test failure with something that is meaningful.

src/segmantic/seg/dataset.py Outdated Show resolved Hide resolved
Copy link
Owner

@dyollb dyollb left a comment

Choose a reason for hiding this comment

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

The changes look good now.

  • Please avoid the code duplication marked in dataset.py
  • and check that training and cross-validate run properly.
  • also add a check that the (training/validation) files in the dataset actually exist and are not just globs (in the modified test).

@dyollb
Copy link
Owner

dyollb commented Jul 12, 2022

LGTM

@dyollb dyollb merged commit 709f976 into dyollb:main Jul 23, 2022
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.

2 participants