Skip to content

RAM #524

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
Merged
tachella merged 59 commits into from
Aug 5, 2025
Merged

RAM #524

tachella merged 59 commits into from
Aug 5, 2025

Conversation

@matthieutrs
Copy link
Collaborator

@matthieutrs matthieutrs commented Jun 9, 2025 •
edited
Loading

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.

@codecov
Copy link

codecov bot commented Jun 9, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 81.15450% with 111 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.03%. Comparing base (25224a4) to head (5b24833).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
deepinv/models/ram.py 79.44% 48 Missing and 26 partials ⚠️
deepinv/tests/test_models.py 77.61% 11 Missing and 4 partials ⚠️
deepinv/tests/test_physics.py 87.64% 11 Missing ⚠️
deepinv/physics/wrappers.py 82.14% 10 Missing ⚠️
deepinv/physics/blur.py 90.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
+ Coverage   83.01%   83.03%   +0.01%     
==========================================
  Files         196      198       +2     
  Lines       18451    19017     +566     
  Branches     2470     2553      +83     
==========================================
+ Hits        15318    15791     +473     
- Misses       2283     2354      +71     
- Partials      850      872      +22

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

@tachella
Copy link
Contributor

tachella commented Jun 10, 2025

do you think you can make a demo file too? maybe with some finetuning at the end?

tachella
tachella reviewed Jun 10, 2025
View reviewed changes
tachella
tachella reviewed Jun 10, 2025
View reviewed changes
tachella
tachella reviewed Jun 10, 2025
View reviewed changes
@matthieutrs
Copy link
Collaborator Author

matthieutrs commented Jun 12, 2025

still todo:

  • add example
  • check docs

tachella
tachella reviewed Jun 17, 2025
View reviewed changes
tachella
tachella reviewed Jun 17, 2025
View reviewed changes
@matthieutrs matthieutrs marked this pull request as ready for review June 24, 2025 11:44
tachella
tachella reviewed Jun 25, 2025
View reviewed changes
tachella
tachella reviewed Jun 25, 2025
View reviewed changes
tachella
tachella reviewed Jun 25, 2025
View reviewed changes
tachella
tachella reviewed Jun 25, 2025
View reviewed changes
tachella
tachella reviewed Jun 25, 2025
View reviewed changes
tachella
tachella reviewed Jun 25, 2025
View reviewed changes
tachella
tachella requested changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@tachella tachella left a comment •
edited
Loading

Choose a reason for hiding this comment

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

many thanks for this!

On top of the suggested changes to the demo to Poisson, I think I would change the way the model is added to the User Guide. I would suggest to add it in the list of Denoisers

deepinv/docs/source/user_guide/reconstruction/denoisers.rst

Line 108 in 18c7067

- Yes

(since it can be used as a denoiser), and also as a 'Specific Networks'

deepinv/docs/source/user_guide/reconstruction/specific.rst

Line 11 in 18c7067

* - Model

although the name specific is somewhat contradictory, and maybe we should call them differently 'Other Models'? Maybe a good option is to keep two subsections, one with Problem-Specific (for eg PanNet) and General Models for DPIR and RAM?

Andrewwango
Andrewwango requested changes Jun 27, 2025
View reviewed changes
Copy link
Collaborator

@Andrewwango Andrewwango left a comment

Choose a reason for hiding this comment

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

CHANGELOG please.

Thanks for integrating RAM into deepinv, here are some comments!

@matthieutrs matthieutrs mentioned this pull request Jul 1, 2025
Closed
@matthieutrs
Copy link
Collaborator Author

matthieutrs commented Jul 4, 2025 •
edited
Loading

Thanks @Andrewwango!

I'm halfway through your comments - I'll ping you when done.

TODO: compare with MultiScalePhysics

@Andrewwango Andrewwango mentioned this pull request Jul 4, 2025
Open
5 tasks
Andrewwango
Andrewwango requested changes Jul 29, 2025
View reviewed changes
Copy link
Collaborator

@Andrewwango Andrewwango left a comment

Choose a reason for hiding this comment

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

Thanks Matthieu! hopefully nearing the end, some comments and questions:

tachella
tachella requested changes Jul 30, 2025
View reviewed changes
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.

some more changes, mostly on docs and efficiency

Andrewwango
Andrewwango requested changes Aug 1, 2025
View reviewed changes
Copy link
Collaborator

@Andrewwango Andrewwango left a comment

Choose a reason for hiding this comment

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

Adding a Request changes to fix performance tests discussed offline.

@Andrewwango
Copy link
Collaborator

Andrewwango commented Aug 1, 2025

I've added the RAM example directly in e89ffaa in my PR #622 and made some changes, so I'm happy to merge this first

This was referenced Aug 2, 2025
Closed
Open
Andrewwango
Andrewwango requested changes Aug 4, 2025
View reviewed changes
@Andrewwango Andrewwango self-requested a review August 4, 2025 09:28
Andrewwango
Andrewwango requested changes Aug 4, 2025
View reviewed changes
jscanvic added a commit to jscanvic/deepinv that referenced this pull request Aug 4, 2025
@jscanvic
squash-merge deepinv#524
08f748f
Andrewwango
Andrewwango approved these changes Aug 5, 2025
View reviewed changes
Copy link
Collaborator

@Andrewwango Andrewwango left a comment

Choose a reason for hiding this comment

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

LGTM

@tachella tachella merged commit b591d4a into main Aug 5, 2025
9 checks passed
@matthieutrs matthieutrs deleted the ram branch August 18, 2025 08:22
jscanvic added a commit to jscanvic/deepinv that referenced this pull request Aug 21, 2025
@jscanvic
squash-merge deepinv#524
ab65541
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Andrewwango Andrewwango Andrewwango approved these changes

@tachella tachella tachella approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@matthieutrs @tachella @Andrewwango