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.

Sign up for GitHub

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

Jump to bottom

Implement new models #228

Merged
merged 28 commits into from
Oct 31, 2022
Merged

Implement new models #228

merged 28 commits into from
Oct 31, 2022

Conversation

georgeyiasemis
Copy link
Contributor

@georgeyiasemis georgeyiasemis commented Oct 24, 2022 •
edited
Loading

Implements new MRI models along with tests:

  • Conjugate Gradient Network - ConjGradNet (along with baseline)
  • Iterative Dual Network - IterDualNet (Similar to JointICNet but no optimisation for sensitivity maps in the model)
  • Simple Residual Network - ResNet, which can be used as a denoiser (replacement of UNet or DIDN, etc)
  • Variable Splitting Network - VarSplitNet, solves the variable splitting optimization problem for MRI using a DL denoiser for the denoising equation and gradient descent for the data consistency equation

Also adds complex_dot_product and complex_division transforms

@github-actions github-actions bot added documentation Improvements or additions to documentation python labels Oct 24, 2022
@georgeyiasemis georgeyiasemis self-assigned this Oct 24, 2022
@codecov
Copy link

codecov bot commented Oct 24, 2022 •
edited
Loading

Codecov Report

Base: 81.91% // Head: 82.92% // Increases project coverage by +1.01% 🎉

Coverage data is based on head (c02fdc9) compared to base (73eb5c6).
Patch coverage: 97.65% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #228      +/-   ##
==========================================
+ Coverage   81.91%   82.92%   +1.01%     
==========================================
  Files          77       88      +11     
  Lines        5589     5967     +378     
==========================================
+ Hits         4578     4948     +370     
- Misses       1011     1019       +8
Impacted Files Coverage Δ
direct/nn/varnet/varnet.py 94.44% <ø> (ø)
direct/types.py 77.77% <64.70%> (-22.23%) ⬇️
direct/nn/varsplitnet/varsplitnet.py 95.89% <95.89%> (ø)
direct/constants.py 100.00% <100.00%> (ø)
direct/data/datasets_config.py 100.00% <100.00%> (ø)
direct/data/mri_transforms.py 97.00% <100.00%> (ø)
direct/data/transforms.py 96.31% <100.00%> (+0.68%) ⬆️
direct/engine.py 75.00% <100.00%> (ø)
direct/nn/conjgradnet/conjgrad.py 100.00% <100.00%> (ø)
direct/nn/conjgradnet/conjgradnet.py 100.00% <100.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

georgeyiasemis
georgeyiasemis commented Oct 26, 2022
View reviewed changes
Copy link
Contributor Author

@georgeyiasemis georgeyiasemis left a comment

Choose a reason for hiding this comment

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

ok

jonasteuwen
jonasteuwen reviewed Oct 26, 2022
View reviewed changes
Copy link
Contributor

@jonasteuwen jonasteuwen left a comment

Choose a reason for hiding this comment

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

A few code quality requests

direct/data/mri_transforms.py Outdated Show resolved Hide resolved
direct/data/transforms.py Show resolved Hide resolved
direct/data/transforms.py Show resolved Hide resolved
direct/nn/conjgradnet/config.py Outdated Show resolved Hide resolved
direct/nn/conjgradnet/conjgrad.py Outdated Show resolved Hide resolved
direct/nn/recurrentvarnet/recurrentvarnet.py Outdated Show resolved Hide resolved
direct/nn/resnet/config.py Show resolved Hide resolved
direct/nn/varsplitnet/varsplitnet.py Outdated Show resolved Hide resolved
direct/nn/varsplitnet/varsplitnet.py Outdated Show resolved Hide resolved
direct/nn/varsplitnet/varsplitnet_engine.py Outdated Show resolved Hide resolved
jonatanferm
jonatanferm reviewed Oct 27, 2022
View reviewed changes
Copy link

@jonatanferm jonatanferm 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 comments/questions.
Don't have time to go through more now. Will look at the rest tomorrow :)

direct/data/mri_transforms.py Show resolved Hide resolved
direct/data/transforms.py Show resolved Hide resolved
direct/nn/build_nn_model.py Outdated Show resolved Hide resolved
direct/nn/conjgradnet/config.py Outdated Show resolved Hide resolved
jonatanferm
jonatanferm reviewed Oct 28, 2022
View reviewed changes
Copy link

@jonatanferm jonatanferm left a comment

Choose a reason for hiding this comment

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

Always hard to review when you are not well versed in the code base :)

In general I think it looks good and it's great to see some tests! Maybe some of the methods could have a couple of more tests though?

The main things I think could be improved are: added documentation to the classes where it is missing, and reduced use of "magic strings" :)

direct/nn/conjgradnet/conjgradnet.py Show resolved Hide resolved
direct/nn/conjgradnet/conjgradnet_engine.py Show resolved Hide resolved
direct/nn/iterdualnet/iterdualnet.py Show resolved Hide resolved
direct/nn/iterdualnet/iterdualnet.py Show resolved Hide resolved
direct/nn/iterdualnet/iterdualnet.py Show resolved Hide resolved
direct/nn/iterdualnet/iterdualnet.py Show resolved Hide resolved
direct/nn/varsplitnet/varsplitnet.py Outdated Show resolved Hide resolved
tests/tests_nn/test_conjgradnet.py Show resolved Hide resolved
@jonasteuwen jonasteuwen self-requested a review October 31, 2022 20:06
jonasteuwen
jonasteuwen reviewed Oct 31, 2022
View reviewed changes
Copy link
Contributor

@jonasteuwen jonasteuwen left a comment

Choose a reason for hiding this comment

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

LGTM

jonasteuwen
jonasteuwen approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@jonasteuwen jonasteuwen left a comment

Choose a reason for hiding this comment

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

LGTM

@jonasteuwen jonasteuwen merged commit e7ea67e into main Oct 31, 2022
@jonasteuwen jonasteuwen deleted the implement-new-models branch October 31, 2022 20:45
georgeyiasemis added a commit that referenced this pull request Oct 31, 2022
@georgeyiasemis
Merge pull request #229 from NKI-AI/main
211748a 
Implement new models (#228)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonatanferm jonatanferm jonatanferm left review comments

@jonasteuwen jonasteuwen jonasteuwen approved these changes

Assignees

@georgeyiasemis georgeyiasemis

Labels
documentation Improvements or additions to documentation python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@georgeyiasemis @jonasteuwen @jonatanferm