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

Fix operator chaining #189

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Fix operator chaining #189

merged 2 commits into from
Feb 29, 2024

Conversation

fzimmermann89
Copy link
Member

@fzimmermann89 fzimmermann89 commented Feb 25, 2024

Two spots I missed when we switched to operators returning tuples:
In the @ chain, we need to unpack the return of the inner operator when calling the outer operator.

Mypy did not catch that as the types on the general operator are not bound to Tensor.

Edit: A couple more issues....
We should maybe consider running mypy on the tests as well...

(Maybe https://stackoverflow.com/questions/78013162/typehint-function-args-tupleargs-with-constrain-on-the-args could at some point help here)

Copy link
Contributor

github-actions bot commented Feb 25, 2024

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/mrpro/algorithms
   _adam.py13838%93–103
   _lbfgs.py14843%68, 85–94
   _remove_readout_os.py30293%46, 48
src/mrpro/data
   _AccelerationFactor.py13130%15–37
   _AcqInfo.py79297%100, 135
   _Data.py15287%58, 73
   _DcfData.py82495%34, 75, 101, 189
   _IData.py57788%44, 49, 109, 114, 122, 128, 132
   _IHeader.py58297%86, 113
   _KData.py1111289%103–104, 200, 202, 232, 237–238, 282, 284, 288, 318, 334
   _KHeader.py125993%34, 82, 120, 171, 182, 189–190, 193, 200
   _KNoise.py21957%56–69
   _KTrajectory.py63297%228, 248
   _SpatialDimension.py39587%76, 79, 86–88
   _TrajectoryDescription.py14193%37
src/mrpro/data/traj_calculators
   _KTrajectoryCalculator.py26388%37, 63, 65
   _KTrajectoryIsmrmrd.py15287%55, 62
   _KTrajectoryPulseq.py33197%72
src/mrpro/operators
   _CartesianSamplingOp.py48981%58–59, 64–65, 70–71, 95, 98, 122
   _ConstraintsOp.py58297%34, 36
   _LinearOperator.py49492%37, 90, 106, 111
   _Operator.py45198%36
   _ZeroPadOp.py15193%45
src/mrpro/utils
   _rgetattr.py3167%20
   _zero_pad_or_crop.py30680%37, 41, 64, 67, 70, 73
TOTAL172911693% 

Tests Skipped Failures Errors Time
189 0 💤 0 ❌ 0 🔥 35.378s ⏱️

@fzimmermann89
Copy link
Member Author

TODO: Check if mypy likes the tests

Copy link
Collaborator

@schuenke schuenke left a comment

Choose a reason for hiding this comment

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

The need to use typing.cast() does not necessarily make the code easier to read, but unfortunately it seems to be unavoidable.

The rest looks fine and I checked already that everything is compatible with the ruff settings from #193

@fzimmermann89 fzimmermann89 merged commit 250d0ad into main Feb 29, 2024
7 checks passed
@fzimmermann89 fzimmermann89 deleted the fix_operatorchain branch February 29, 2024 16:20
fzimmermann89 added a commit that referenced this pull request Nov 10, 2024
Fixes returning a tuple, the linear operator adjoint, typing, and adds more tests.
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.

2 participants