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

Small improvements to unet.UNet, shift.Shift & noise.PoissonNoise #137

Merged
merged 12 commits into from
Jan 10, 2024

Conversation

Andrewwango
Copy link
Collaborator

@Andrewwango Andrewwango commented Dec 21, 2023

  • UNet: option to select nn.BatchNorm2d as batch norm instead of BFBatchNorm2d
  • Shift: add option for max percentage shift
  • PoissonNoise: clip input to non-negative

Checks to be done before submitting your PR

  • python3 -m pytest tests/ runs successfully.
  • black . runs successfully.
  • make html runs successfully (in the docs/ directory).
  • Updated docstrings related to the changes (as applicable).
  • Added an entry to the CHANGELOG.rst.

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3d2abb8) 70.57% compared to head (d1972ed) 70.59%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #137       /-   ##
==========================================
  Coverage   70.57%   70.59%    0.01%     
==========================================
  Files          93       93              
  Lines        6573     6611       38     
  Branches      894      898        4     
==========================================
  Hits         4639     4667       28     
- Misses       1631     1651       20     
  Partials      303      293      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@matthieutrs matthieutrs left a comment

Choose a reason for hiding this comment

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

Hi, thanks a lot for the suggestions! See my comments above :)

deepinv/models/unet.py Outdated Show resolved Hide resolved
deepinv/physics/noise.py Outdated Show resolved Hide resolved
deepinv/transform/shift.py Outdated Show resolved Hide resolved
Co-authored-by: Matthieu Terris <31830373 [email protected]>
Copy link
Contributor

@tachella tachella left a comment

Choose a reason for hiding this comment

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

Sorry for the very late review!

The PR looks good, I just worry that the BFBatchNorm2d module from Mohan et al sometimes produces NaNs during training which causes the whole training procedure to fail.

@Andrewwango did you observe this as well? Maybe there is a solution

deepinv/physics/noise.py Outdated Show resolved Hide resolved
deepinv/transform/shift.py Outdated Show resolved Hide resolved
deepinv/models/unet.py Show resolved Hide resolved
Copy link
Contributor

@tachella tachella left a comment

Choose a reason for hiding this comment

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

thanks a lot for the PR! looks good to go - I will merge if it passes the tests

@tachella tachella merged commit 98df7a7 into deepinv:main Jan 10, 2024
5 checks passed
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.

3 participants