Lbfgs box

[schuenke]

closes #115

[github-actions]

Coverage Report

File	Stmts	Miss	Cover	Missing
src/mrpro/algorithms
_adam.py	13	8	38%	93–103
_lbfgs.py	14	8	43%	68, 85–94
_remove_readout_os.py	30	2	93%	46, 48
src/mrpro/data
_AccelerationFactor.py	13	13	0%	15–37
_AcqInfo.py	64	1	98%	93
_Data.py	15	2	87%	58, 73
_DcfData.py	82	4	95%	34, 75, 101, 189
_IData.py	57	7	88%	44, 49, 109, 114, 122, 128, 132
_IHeader.py	58	2	97%	86, 113
_KData.py	111	12	89%	103–104, 200, 202, 232, 237–238, 282, 284, 288, 318, 334
_KHeader.py	125	9	93%	34, 82, 120, 171, 182, 189–190, 193, 200
_KNoise.py	21	9	57%	56–69
_KTrajectory.py	63	2	97%	228, 248
_SpatialDimension.py	39	5	87%	76, 79, 86–88
_TrajectoryDescription.py	14	1	93%	37
src/mrpro/data/traj_calculators
_KTrajectoryCalculator.py	26	3	88%	37, 63, 65
_KTrajectoryIsmrmrd.py	15	2	87%	55, 62
_KTrajectoryPulseq.py	33	1	97%	72
src/mrpro/operators
_CartesianSamplingOp.py	48	9	81%	58–59, 64–65, 70–71, 95, 98, 122
_ConstraintsOp.py	58	2	97%	34, 36
_LinearOperator.py	49	8	84%	37, 72, 80, 88–90, 106, 111
_Operator.py	43	1	98%	35
_ZeroPadOp.py	15	1	93%	45
src/mrpro/utils
_rgetattr.py	3	1	67%	20
_zero_pad_or_crop.py	30	6	80%	37, 41, 64, 67, 70, 73
TOTAL	1712	119	93%

Tests	Skipped	Failures	Errors	Time
186	0 💤	0 ❌	0 🔥	53.347s ⏱️

[fzimmermann89]

Maybe we should copy the initial value tensor?

It might be unexpected that the tensor is changed during the runtime..

[fzimmermann89]

mypy fails with:

src/mrpro/operators/_ConstraintsOp.py:22: error: Bad number of arguments, expected: at least 1, given: 0  [type-arg]
src/mrpro/operators/functionals/_mse_data_discrepancy.py:21: error: Bad number of arguments, expected: at least 1, given: 0  [type-arg]

the tests fail because of #174 (not our fault, also tracked on ismrmrd python repo)

[ckolbPTB]

mypy fails with:

src/mrpro/operators/_ConstraintsOp.py:22: error: Bad number of arguments, expected: at least 1, given: 0  [type-arg]
src/mrpro/operators/functionals/_mse_data_discrepancy.py:21: error: Bad number of arguments, expected: at least 1, given: 0  [type-arg]

Can we use something like "# type: ignore [type-arg]" here to satisfy mypy if this is due to a problem in 3rd party code?

[fzimmermann89]

• There are some typing issues I think. The operator types are not optimal, see my comments above.
  The functionals can be typed as returning only a single tensor.
• softplus can soon take in a float in pytorch. I reported the bug in their typing (the code always worked with float) and it got fixed. maybe we can already type beta as float and set #type: ignore SOMETHING to ignore the error ..

Otherwise, I think the code makes sense. You also convinced my that it makes sense to have the with pytest.raises there as a reminder to remove it once this line does not raise an error any more.

[fzimmermann89]

I addressed some of the review comments regarding the comments ( ;-) ) and added tests for what I think should be illegal bounds.

        ((1.0, -1.0), (-1.0, 1.0)),  # first one is invalid
        ((-1.0, 1.0), (1.0, -1.0)),  # second one is invalid
        ((1.0, 1.0),),   ### QUESTION: IS THIS VALID OR INVALID? ####
        ((torch.nan, 1),),
        ((-1, torch.nan),),
        ((torch.inf, -torch.inf),),

Some of these seem not to raise an exception in the constraintOp. If they are actually valid, feel free to remove them from the test.
Also, do we want to allow inf and -inf for bounds? This feels (for me) more natural to write than None for an unbounded variable.

[fzimmermann89]

@koflera :
Maybe clean up the commit message next time you merge a PR...