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

NF: Updating EVAC+ model and adding util function #2963

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

pjsjongsung
Copy link
Contributor

This commit changes the EVAC+ model and its default weights for better brain extraction output. It also adds an option to only select the largest foreground and remove its holes, so that most prediction noises can be removed.

@pep8speaks
Copy link

pep8speaks commented Nov 3, 2023

Hello @pjsjongsung, Thank you for updating !

Line 48:71: W292 no newline at end of file

Comment last updated at 2023-11-15 18:07:44 UTC

@pjsjongsung pjsjongsung force-pushed the evac_new_student branch 4 times, most recently from cb2c8a7 to 6c63841 Compare November 4, 2023 15:00
Copy link

codecov bot commented Nov 4, 2023

Codecov Report

Merging #2963 (4a2a68b) into master (618b395) will decrease coverage by 0.01%.
Report is 9 commits behind head on master.
The diff coverage is 98.07%.

❗ Current head 4a2a68b differs from pull request most recent head 529aa01. Consider uploading reports for the commit 529aa01 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2963      +/-   ##
==========================================
- Coverage   81.71%   81.70%   -0.01%     
==========================================
  Files         147      147              
  Lines       20484    20540      +56     
  Branches     3277     3285       +8     
==========================================
+ Hits        16739    16783      +44     
- Misses       2913     2924      +11     
- Partials      832      833       +1     
Files Coverage Δ
dipy/data/fetcher.py 40.25% <ø> (ø)
dipy/nn/utils.py 96.61% <100.00%> (+4.94%) ⬆️
dipy/nn/evac.py 82.48% <92.30%> (+0.98%) ⬆️

... and 2 files with indirect coverage changes

@pjsjongsung
Copy link
Contributor Author

I think the PR looks ok? The failures seems to be coming from unrelated tests that uses numpy random, which I plan to fix through #2964

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @pjsjongsung,

see below an initial reveiw before I test the code. Thank you for this!

dipy/nn/evac.py Outdated Show resolved Hide resolved
dipy/nn/evac.py Outdated Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
@pjsjongsung pjsjongsung force-pushed the evac_new_student branch 3 times, most recently from 60e2b69 to 4a2a68b Compare November 10, 2023 04:59
@pjsjongsung
Copy link
Contributor Author

The comments should be all resolved!

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @pjsjongsung,

This PR seems ready to go. See below my last comments.

dipy/data/fetcher.py Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
dipy/nn/utils.py Outdated Show resolved Hide resolved
This commit changes the EVAC+ model and its default weights for better
brain extraction output. It also adds an option to only select the
largest foreground and remove its holes, so that most prediction noises
can be removed. Slice wise filling holes were added as well for more
desired results. Utils functions have changed to use voxsize, and tests
have changed accordingly as well.
Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Thank you @pjsjongsung,

LGTM, merging

@skoudoro skoudoro merged commit 9ec381c into dipy:master Nov 15, 2023
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants