Fix:missing-t_ktraj-return-from-calculate_kspace

[Nikbert]

This adds t_ktraj to the return values of calculate_kspace, to make it consistent with the Matlab Pulseq Version. t_ktraj is very handy when comparing measured and nominal trajectories.

All the Best

[github-actions[bot]]

Coverage Report

File	Stmts	Miss	Cover	Missing
/home/runner/.local/lib/python3.12/site-packages/pypulseq
add_gradients.py	123	51	59%	44, 52, 58, 61, 75–86, 92, 120–123, 130–131, 150, 157, 162–241
add_ramps.py	36	36	0%	1–89
align.py	35	4	89%	41, 45, 69, 73
calc_duration.py	25	1	96%	37
calc_ramp.py	218	214	2%	45–353
calc_rf_bandwidth.py	27	20	26%	37–59, 63–67
check_timing.py	87	27	69%	72, 76, 101, 194, 201, 211–255
compress_shape.py	30	1	97%	28
convert.py	40	8	80%	42, 48, 66, 72–73, 82, 88–89
event_lib.py	96	14	85%	6–9, 48–51, 70–71, 205–210
make_adc.py	92	13	86%	63, 72–76, 79, 128, 131, 135, 141, 145, 184, 186, 188, 196
make_adiabatic_pulse.py	129	39	70%	196–200, 217–221, 229–230, 253, 259, 328–347, 451–460, 498–506
make_arbitrary_grad.py	37	7	81%	68, 71, 74, 77, 81, 83, 103
make_arbitrary_rf.py	66	55	17%	83–160
make_block_pulse.py	46	3	93%	112–116, 119
make_delay.py	9	1	89%	27
make_digital_output_pulse.py	16	2	88%	39, 47
make_extended_trapezoid.py	56	12	79%	67, 70, 76, 82, 85, 88, 91, 94, 116, 134, 136, 139
make_extended_trapezoid_area.py	93	3	97%	52, 227, 230
make_gauss_pulse.py	69	20	71%	127–131, 134–158, 165, 168
make_label.py	19	3	84%	36, 41, 43
make_sigpy_pulse.py	116	31	73%	12–13, 112, 115, 119, 154, 157–161, 165, 168–169, 172–173, 188, 195, 200, 212, 215, 240–250, 264, 267, 297–307
make_sinc_pulse.py	68	10	85%	94, 100, 127–131, 135, 138–139, 142–143, 165
make_trapezoid.py	111	7	94%	177, 190, 196, 214, 232, 237, 255
make_trigger.py	16	2	88%	44, 52
opts.py	66	9	86%	78, 83, 102, 142, 166–170
points_to_waveform.py	9	1	89%	27
rotate.py	69	14	80%	15, 55, 66–69, 85–90, 112, 119–120
scale_grad.py	14	4	71%	28–30, 33
sigpy_pulse_opts.py	26	7	73%	34–41
split_gradient.py	39	31	21%	46–103
split_gradient_at.py	70	27	61%	63–90, 110, 114, 118–120, 154–156
traj_to_grad.py	13	9	31%	26–40
/home/runner/.local/lib/python3.12/site-packages/pypulseq/SAR
SAR_calc.py	113	98	13%	33–40, 55–62, 89–108, 129–132, 168–212, 242–246, 264–306
/home/runner/.local/lib/python3.12/site-packages/pypulseq/Sequence
block.py	368	32	91%	59, 62, 70, 76, 91, 99, 105, 116, 119, 122, 130, 135, 144, 196, 245, 249, 265, 305–308, 337–338, 404, 410, 430, 499, 535, 541, 568, 606, 640
calc_grad_spectrum.py	81	76	6%	68–190
calc_pns.py	40	31	22%	45–96
ext_test_report.py	140	11	92%	23, 58, 61, 135, 227–233
install.py	75	42	44%	31, 52, 69, 71, 112–131, 148, 181–184, 200–212, 254–278
parula.py	4	2	50%	19–86
read_seq.py	312	68	78%	42–43, 90, 93, 105, 110, 116, 123, 132, 141, 146, 149, 157–159, 192, 197, 205–254, 284–287, 302–303, 332–349, 412, 415, 450, 458, 532, 574–578
sequence.py	737	224	70%	11–14, 101–111, 132–145, 183, 248–251, 300, 327, 344, 392, 420, 447–452, 489, 505, 596, 618, 659–662, 716, 754, 765–766, 772, 783, 789, 791, 799, 832–840, 861–883, 926, 928, 931, 957–958, 961–964, 1000–1010, 1019–1021, 1065–1077, 1092–1093, 1129–1130, 1156, 1162, 1165, 1202, 1323–1336, 1359, 1387, 1409–1411, 1432, 1463, 1474–1487, 1499–1510, 1556–1557, 1566–1584, 1608, 1638–1646, 1678–1788, 1817, 1831–1841, 1853
write_seq.py	172	8	95%	41, 65, 68–75
/home/runner/.local/lib/python3.12/site-packages/pypulseq/utils
cumsum.py	14	1	93%	17
safe_pns_prediction.py	126	113	10%	50–87, 102–189, 197–214, 222, 244–250, 279–286, 310–336, 344–383, 396–411, 415
tracing.py	16	6	62%	33–34, 42, 54–55, 75
/home/runner/.local/lib/python3.12/site-packages/pypulseq/utils/siemens
asc_to_hw.py	58	53	9%	21–28, 48–106
readasc.py	48	45	6%	25–100
TOTAL	4372	1496	66%

Tests	Skipped	Failures	Errors	Time
1285	18 :zzz:	0 :x:	0 :fire:	2m 57s :stopwatch:

[schuenke]

As far as I can see, the Matlab example sequences usually use the calculateKspacePP function, which returns even more parameters like t_adc and slicepos: https://github.com/pulseq/pulseq/blob/5ccd39c4170e48eec8542162ce7b4ec0839d91c1/matlab/%2Bmr/%40Sequence/Sequence.m#L2013

There is also (an old?) calculateKspace function, which, similar to the Python method, returns just 5 values: https://github.com/pulseq/pulseq/blob/5ccd39c4170e48eec8542162ce7b4ec0839d91c1/matlab/%2Bmr/%40Sequence/Sequence.m#L1344

So in my opinion we should keep some consistency and either leave the Python function as it is or adjust it to match the Matlab calculateKspacePP function

[FrankZijlstra]

I ran into this when I reimplemented calculate_kspace at the time. I agree that it is a useful return value, but any external script that uses calculate_kspace will break as a result.

One non-breaking option is to add a return_t_ktraj parameter, default to False, and give a deprecation warning when it is said to False (i.e. in some next release the old-style return values will be removed). When it is True, return the new return values as in the PR.

[Nikbert]

I think your suggestions makes sense. So, are we going to adapt the strange naming "calculateKspacePP" for a new function that has all the return values, mirroring the way it is done in pulseq? (I talked to some colleagues here in Freiburg, everyone is using calculateKspacePP.) Or should we do it as Franks suggested? I think in pulseq calculateKspacePP is planed to replace calculateKspace in the future as well. I somewhat lean to implementing "calculateKspacePP" as it makes it easier for someone who uses both implementations. However, it is an ugly solution as it leaves use with two very similar functions... Any comments?

[FrankZijlstra]

calculateKspacePP refers to the piecewise-polynomial implementation for k-space calculation. We already moved this to calculate_kspace, because it is the only implementation that should be used. calculate_kspacePP exists in pypulseq merely for backwards compatibility (it will give a deprecation warning when used), but should be removed at some point. So no, this function should not be touched.

[Nikbert]

OK, then we can not mirror Pulseq calculateKspacePP. I think Franks suggestion sounds like a good solution. I think everyone that wants to match k-space data from a Skope fieldcam to the nominal trajectory would highly appreciate this change. I do not know how to alter this pull request accordingly. Frank can you help me?

[btasdelen]

If we are going to introduce a breaking change with an option to roll back via a return_XYZ parameter, would it make sense to return a dict instead? This function is returning 5 values (6 with the new addition), not really clean.

[FrankZijlstra]

I don't think it makes sense to have the rollback option not set as a default, because then it would still break old code unless the new parameter is set. With it set to default we can give a deprecation warning.

If we break things, I agree that returning a dict is nicer. Though a SimpleNamespace or result class (e.g. as sometimes used in scipy and numpy) is probably ever nicer. @schuenke probably can tell what is preferred if we do go that way.

To be clear, I'm not opposed to a breaking change, because even when we deprecate the current result values, at some point the breaking change needs to happen anyway. But this is something that would need to be documented. And a user should not get a "too many values to unpack" error without any indication why.