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]

Coverage Report

File	Stmts	Miss	Cover	Missing
/home/runner/.local/lib/python3.12/site-packages/pypulseq
add_gradients.py	137	60	56%	44, 52, 58, 61, 75–86, 92, 125–128, 135–136, 155, 162, 167–263
add_ramps.py	36	36	0%	1–92
align.py	35	4	89%	41, 45, 69, 73
calc_duration.py	33	2	94%	43, 50
calc_ramp.py	220	216	2%	48–359
calc_rf_bandwidth.py	37	28	24%	45–81, 85–89
check_timing.py	96	29	70%	78, 82, 107, 180, 199, 232, 239, 249–293
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	98	14	86%	77, 80, 90–94, 97, 146, 149, 153, 159, 163, 202, 204, 206, 214
make_adiabatic_pulse.py	132	39	70%	208–212, 232–236, 244–245, 268, 274, 343–362, 466–475, 513–521
make_arbitrary_grad.py	53	15	72%	71, 74, 77, 80, 96–98, 107, 109, 117–121, 130
make_arbitrary_rf.py	75	63	16%	95–179
make_block_pulse.py	48	3	94%	121–125, 128
make_delay.py	9	1	89%	27
make_digital_output_pulse.py	16	2	88%	42, 50
make_extended_trapezoid.py	56	12	79%	67, 70, 76, 82, 85, 88, 91, 94, 116, 134, 136, 139
make_extended_trapezoid_area.py	104	4	96%	68, 71, 261, 264
make_gauss_pulse.py	73	20	73%	142–146, 149–173, 180, 183
make_label.py	22	4	82%	64, 66, 68, 75
make_sigpy_pulse.py	119	30	75%	12–13, 122, 125, 129, 166–170, 174, 177–178, 181–182, 197, 204, 209, 221, 224, 249–259, 273, 276, 306–316
make_sinc_pulse.py	70	10	86%	104, 110, 138–142, 146, 149–150, 153–154, 176
make_soft_delay.py	26	2	92%	107, 125
make_trapezoid.py	111	7	94%	177, 190, 196, 214, 232, 237, 255
make_trigger.py	16	2	88%	47, 55
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	30	1	97%	65
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	5	1	80%	9
/home/runner/.local/lib/python3.12/site-packages/pypulseq/Sequence
block.py	469	79	83%	63, 66, 74, 80, 95, 103, 109, 120, 123, 126, 134, 139, 148, 159, 167, 207, 209, 213, 225, 274, 278, 294, 319–345, 382–385, 421–429, 436, 466–470, 512, 518, 551, 587–594, 611, 621, 647, 685, 703, 706, 724, 738, 765, 844, 881, 905
calc_grad_spectrum.py	81	76	6%	68–190
calc_pns.py	40	31	22%	45–96
calculate_kspace.py	89	3	97%	55, 136, 201
ext_test_report.py	169	11	93%	81, 158, 169–170, 339–345
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	400	39	90%	44–45, 110, 117, 130, 133–134, 138, 184, 228, 401, 422–439, 502, 505, 590, 614, 654, 685, 701–705, 712, 823, 834
sequence.py	562	123	78%	10–13, 106–116, 137–150, 204, 269–272, 385, 413–416, 507, 535–544, 556, 578, 619–622, 676, 714, 725–726, 732, 743, 749, 751, 759, 792–800, 932, 1030, 1036, 1039, 1042, 1079, 1204–1217, 1275, 1297–1299, 1320, 1383, 1391, 1489, 1500–1513, 1582–1583, 1594–1612, 1636, 1666–1674, 1710, 1724–1734
write_seq.py	362	7	98%	45, 77–79, 313, 347–349, 499
/home/runner/.local/lib/python3.12/site-packages/pypulseq/utils
cumsum.py	14	1	93%	17
paper_plot.py	63	58	8%	49–132
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
seq_plot.py	407	186	54%	22, 106, 138–139, 159, 176–178, 183–225, 235–242, 249–314, 318–324, 328–336, 359, 361, 363, 390–412, 457–458, 461–464, 487–496, 508, 525–535, 544–546, 565–567, 569–570, 572–573, 657–658, 674–691, 739–743
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	59	7	88%	103–104, 110–111, 122–124
TOTAL	5215	1563	70%

Tests	Skipped	Failures	Errors	Time
1291	24 💤	0 ❌	0 🔥	5m 7s ⏱️

[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.

[lukeje]

I also found I needed this parameter and so rolled up a quick solution in line with what Bilal suggested here which seems to pass the standard tests. Is it worth me making a new pull request or merging it with this one somehow?

[schuenke]

I will have a look at this again. Probably tuesday next week.

[mzaiss]

we see some other k-space inconsistencies, but it would be better if this PR is solved in 1.4 and 1.5 so we can rule some things out. so thumbs up for a solution to this, we are also happy to help here.

[schuenke]

okay, I agree it makes sense to keep the default behaviour as it is so that we dont break existing code. IMO, there are two options now:

1. introduce a return_t_ktraj: bool = False flag and return 6 instead of 5 values if it's set to True
2. introduce a return_all_values_as_dict: bool = False flag and return a dict with 6 (and potentially even more) entries.

What's your preference @FrankZijlstra @mzaiss @lukeje @btasdelen @Nikbert

Ofc the variable names are up for discussion as well

[lukeje]

2. introduce a return_all_values_as_dict: bool = False flag and return a dict with 6 (and potentially even more) entries.

I would prefer this option (as implemented in my branch here: https://github.com/lukeje/pypulseq/tree/calculate_kspace_dict but with output_dict as the variable).

[mzaiss]

2

[schuenke]

Okay, I implemented option 2 in c8cb776

As always when touching some files, I also did some additional refactoring. The most important things:

• define some internal helper functions (b52c5cd)
• change many variable names to be more descriptive (b52c5cd)
• move the whole calculate_kspace code into a separate file (similar to test_report etc) (5f4ee89)
• resolve merge conflics (a9cbc70)
• add some tests (currently only for the new feature) (df6f783)

It should be easy to track the changes if you go through them step by step.

It would be great if @Nikbert and maybe one more maintainer could review the changes. I prefer to avoid approving my own implementations / changes.

[lukeje]

This updated branch works for me. Though I noticed that calculate_kspace is also used in tests/test_sequence.py, e.g. in line 484. If the plan is to deprecate the old form, it might be good to update these calls too.

[schuenke]

Thanks for your feedback. I will have a look at the existing function calls and might update them accordingly.

[Nikbert]

Looks good to me! Thanks for following up on this issue.