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

Add Wavelet Operator #294

Merged
merged 30 commits into from
Sep 11, 2024
Merged

Add Wavelet Operator #294

merged 30 commits into from
Sep 11, 2024

Conversation

schuenke
Copy link
Collaborator

@schuenke schuenke commented May 24, 2024

Wavelet Operator for wavelet-reco

For wavelet transforms we use The PyTorch Wavelet Toolbox (ptwt), which extends PyWavelets (pywt). It provides GPU and gradient support via a PyTorch backend.

Copy link
Contributor

github-actions bot commented May 24, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms/csm
   inati.py24196%44
   iterative_walsh.py15193%37
src/mrpro/algorithms/dcf
   dcf_voronoi.py53492%15, 48–49, 76
src/mrpro/algorithms/optimizers
   adam.py20195%69
src/mrpro/algorithms/reconstruction
   DirectReconstruction.py281643%51–71, 85
   IterativeSENSEReconstruction.py422345%77–78, 88–98, 113–124, 138–149
   Reconstruction.py512453%41, 53–55, 79–86, 103–114
src/mrpro/data
   AcqInfo.py128298%174, 214
   CsmData.py28389%14, 84–86
   DcfData.py44882%17, 65, 77–82
   IData.py67987%119, 125, 129, 159–167
   IHeader.py75791%75, 109, 127–131
   KHeader.py1641790%24, 126–130, 157, 207, 218, 225–226, 229, 236, 275–286
   KNoise.py311552%39–52, 56–61
   KTrajectory.py69593%178–182
   MoveDataMixin.py1261489%14, 109, 125, 139–141, 202, 265, 279, 358, 378–379, 396–397
   QData.py39782%42, 65–73
   SpatialDimension.py46296%64, 103
   TrajectoryDescription.py14193%23
   acq_filters.py10190%47
src/mrpro/data/_kdata
   KData.py1051685%107–108, 117, 125, 179–180, 215, 220–221, 240–251
   KDataRemoveOsMixin.py29293%43, 45
   KDataSelectMixin.py20290%46, 62
   KDataSplitMixin.py48394%49, 79, 88
src/mrpro/data/traj_calculators
   KTrajectoryCalculator.py25292%23, 45
   KTrajectoryIsmrmrd.py13285%41, 50
   KTrajectoryPulseq.py29197%54
src/mrpro/operators
   CartesianSamplingOp.py50982%49–50, 55–56, 61–62, 88, 91, 114
   ConstraintsOp.py60297%46, 48
   EndomorphOperator.py51296%209, 213
   FiniteDifferenceOp.py27293%48, 113
   FourierOp.py77199%131
   GridSamplingOp.py123993%60–61, 70–71, 78–79, 82, 84, 86
   LinearOperator.py80495%32, 131, 251, 256
   Operator.py52198%21
   SliceProjectionOp.py166895%39, 46, 48, 54, 191, 212, 245, 285
   WaveletOp.py120596%152, 170, 205, 210, 233
   ZeroPadOp.py16194%30
src/mrpro/utils
   Rotation.py4532894%58–66, 106, 283, 368, 370, 397, 452, 457, 460, 475, 492, 497, 640, 645, 648, 664, 668, 742, 744, 752–753, 993, 1075
   filters.py62297%44, 49
   slice_profiles.py45687%18, 34, 111–114, 147
   sliding_window.py34197%34
   split_idx.py10280%43, 47
   summarize_tensorvalues.py11918%20–29
   zero_pad_or_crop.py31681%26, 30, 54, 57, 60, 63
TOTAL360828792% 

Tests Skipped Failures Errors Time
825 0 💤 0 ❌ 0 🔥 1m 13s ⏱️

@ckolbPTB ckolbPTB requested a review from fzimmermann89 May 30, 2024 19:32
@ckolbPTB ckolbPTB marked this pull request as ready for review May 30, 2024 19:32
@ckolbPTB
Copy link
Collaborator

I get the following error:

worker 'gw3' crashed while running 'tests/data/test_movedatamixin.py::test_movedatamixin_convert[True-dtype1-single]'

As I did not change anything there, I was wondering if @fzimmermann89 has any ideas what this could be about?

tests/helper.py Outdated Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Outdated Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Outdated Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Outdated Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Outdated Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
@fzimmermann89
Copy link
Member

Out of curiosity: pytorch wavelet toolbox changed some type hints as we suggested (on their current GitHub version)

I don't remember why we cared - does this help?

Or will this now cause issues with the current waveletop code?

@ckolbPTB
Copy link
Collaborator

Out of curiosity: pytorch wavelet toolbox changed some type hints as we suggested (on their current GitHub version)
I don't remember why we cared - does this help?
Or will this now cause issues with the current waveletop code?

As far as I can see this fits to your typehinting in the code so it should not be a problem.

@ckolbPTB ckolbPTB requested review from fzimmermann89 and koflera July 2, 2024 10:30
tests/helper.py Outdated Show resolved Hide resolved
Copy link
Member

@fzimmermann89 fzimmermann89 left a comment

Choose a reason for hiding this comment

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

some comments

@fzimmermann89 fzimmermann89 changed the title Wavelet Add Wavelet Operator Jul 12, 2024
@ckolbPTB ckolbPTB self-assigned this Sep 2, 2024
@schuenke schuenke self-assigned this Sep 3, 2024
@schuenke
Copy link
Collaborator Author

schuenke commented Sep 5, 2024

For testing the implementation we used some real scanner data already. Should we create an example script / notebook from our test case and directly include it in this PR?

@ckolbPTB
Copy link
Collaborator

ckolbPTB commented Sep 5, 2024

Should we create an example script / notebook from our test case and directly include it in this PR?

The example we had during the hackathon was already for a FISTA + Wavelet reconstruction. I would therefore suggest to add this example to the FISTA PR.

@fzimmermann89
Copy link
Member

@CodiumAI-Agent /review
--pr_reviewer.extra_instructions="
In the possible issues section, emphasize the following:

  • Is the code logic efficient?
  • Is it idiopathic pytorch code?
  • Are all variable und function names clear and descriptive?
  • Are additional comments required to understand the logic?
  • Check the docstrings for completeness
    "
    --pr_reviewer.inline_code_comments=true
    --pr_reviewer.require_score_review=true
    --pr_reviewer.num_code_suggestions="8"

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Sep 10, 2024

PR Reviewer Guide 🔍

(Review updated until commit 058e475)

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🏅 Score: 85
🧪 PR contains tests
🔒 No security concerns identified
⚡ Key issues to review

Efficiency Concern
The implementation of the wavelet transform might not be efficient due to repeated calculations and potential redundancy in data handling, especially in the forward and adjoint methods where complex data is handled separately.

Variable Clarity
The variable names coeff_ptwt and coeff_mrpro in the test cases might not be immediately clear to all developers. Consider renaming these to more descriptive terms that reflect their roles in the wavelet transformation process.

src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
@ckolbPTB ckolbPTB merged commit f64add5 into main Sep 11, 2024
17 checks passed
@ckolbPTB ckolbPTB deleted the wavelet branch September 11, 2024 18:12
@CodiumAI-Agent
Copy link

Persistent review updated to latest commit 058e475


# Switch off formatter to avoid having only one wavelet type per line
# fmt: off
WaveletType = Literal[

Choose a reason for hiding this comment

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

Consider using a dictionary or a more structured data type for managing wavelet types rather than a long list of literals. This could improve readability and maintainability. [important]

src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
src/mrpro/operators/WaveletOp.py Show resolved Hide resolved
fzimmermann89 added a commit that referenced this pull request Nov 10, 2024
Co-authored-by: ckolbPTB <[email protected]>
Co-authored-by: Felix F Zimmermann <[email protected]>
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.

5 participants