[Nikbert]

closes #281

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	21	85%	44, 52, 61, 85, 92, 125–128, 155, 162, 177, 187–190, 211–218, 225, 230, 261
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	99	28	72%	78, 82, 184, 203, 236, 243, 253–297
compress_shape.py	30	1	97%	28
convert.py	40	8	80%	42, 48, 66, 72–73, 82, 88–89
event_lib.py	96	15	84%	6–9, 48–51, 70–71, 185, 205–210
make_adc.py	108	10	91%	114, 163, 166, 170, 176, 180, 219, 221, 223, 231
make_adiabatic_pulse.py	150	39	74%	214–218, 238–242, 250–251, 274, 280, 349–368, 472–481, 519–527
make_arbitrary_grad.py	62	2	97%	124, 145
make_arbitrary_rf.py	83	24	71%	145–149, 152–156, 166–185, 188, 191
make_block_pulse.py	53	1	98%	132
make_delay.py	9	1	89%	27
make_extended_trapezoid.py	64	6	91%	74, 77, 137, 154, 156, 159
make_extended_trapezoid_area.py	110	4	96%	71, 74, 273, 276
make_gauss_pulse.py	90	5	94%	193, 201–203, 218
make_sigpy_pulse.py	129	28	78%	12–13, 134, 137, 186–190, 197–198, 201–202, 217, 224, 229, 267, 270, 295–305, 345, 348, 378–388
make_sinc_pulse.py	84	4	95%	196–198, 213
make_soft_delay.py	32	1	97%	136
make_trapezoid.py	128	7	95%	207, 221, 227, 245, 263, 268, 286
opts.py	66	9	86%	78, 83, 102, 142, 166–170
points_to_waveform.py	9	1	89%	27
rotate.py	69	5	93%	55, 69, 112, 119–120
rotate_3d.py	78	2	97%	145–146
sigpy_pulse_opts.py	28	7	75%	66–73
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	452	54	88%	64, 67, 75, 81, 96, 104, 110, 121, 124, 127, 135, 140, 149, 160, 168, 208, 210, 214, 329, 333, 336, 357, 372, 382, 393, 402, 434, 438, 447, 460, 471, 480–484, 502, 534–541, 558, 568, 594, 632, 671, 685, 712, 791, 828, 852
calc_grad_spectrum.py	81	76	6%	68–190
calc_pns.py	40	31	22%	45–96
calculate_kspace.py	92	3	97%	58, 139, 204
ext_grad_check.py	55	2	96%	51, 53
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	408	41	90%	44–45, 110, 117, 130, 133–134, 138, 184, 223–224, 242, 415, 436–453, 516, 519, 604, 628, 668, 699, 715–719, 726, 837, 848
sequence.py	637	128	80%	10–13, 109–119, 140–153, 207, 272–275, 549, 577–580, 671, 699–708, 720, 742, 783–786, 815, 841–847, 882, 893–894, 900, 911, 917, 919, 927, 960–968, 1100, 1198, 1204, 1207, 1210, 1249, 1374–1387, 1445, 1467–1469, 1490, 1553, 1561, 1659, 1670–1683, 1752–1753, 1764–1782, 1806, 1836–1844, 1880, 1894–1904
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 Warning: Comment truncated due to GitHub's 65536 character limit

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

[schuenke]

Since its been a while and I wasn't sure myself if everything was ported correctly, I let Claude do another review that might help whoever wants to review this PR:

Here you go:

---

PR #282 Review: calculate_kspace refactoring + t_ktraj return

I did a line-by-line comparison of the old inline method (master sequence.py lines 283–458) against the new standalone calculate_kspace.py. The code transfer is clean — no logic was lost. All 195 relevant tests pass.

Changes overview (5 files, +366 / −151)

File	Change
Sequence/calculate_kspace.py	New file — extracted code with helper functions, renamed variables, added output_as_dict
Sequence/sequence.py	Thin wrapper delegating to ext_calculate_kspace
Sequence/ext_test_report.py	Updated to use output_as_dict=True
tests/test_calculate_kspace.py	New file — 3 tests for legacy, dict, and deprecated path
tests/test_sequence.py	3 call sites updated to use output_as_dict=True

Line-by-line comparison: nothing lost

All logic was preserved with only variable renames for clarity:

Old (master)	New (PR)
gw_pp	grad_waveforms_pp
gm_pp	grad_moments_pp
ng	n_grad_channels
tc	time_candidates
ii	ramp_idx / idx_in_support
dk	k_offset
ii_next_excitation	next_excitation_idx
ii_next_refocusing	next_refocusing_idx
self.*	seq.* (standalone function)

The three helper functions (_build_time_axis, _sample_gradient_moments, _apply_excitation_and_refocusing_resets) are clean extractions of the original inline code.

Improvements found during transfer

1. Bugfix in warning message — The old code had error={t_ktraj(i_period) - t_excitation(ii_next_excitation)} using () (function call syntax) instead of [] (indexing). This would have crashed with a TypeError if the warning was ever triggered. The new code correctly uses [].

2. Removed dead commented-out code — # dk = -k_traj[:, i_period] was cleaned up.

3. Robustified empty-list handling — Old if tc != []: replaced with if len(time_candidates) > 0: plus explicit else: time_candidates = np.zeros(0). Functionally equivalent but cleaner.

Things worth noting (not bugs, just observations)

1. slice_pos is computed but never returned — This exists in both old and new code. The MATLAB calculateKspacePP does return slicepos. Since output_as_dict was introduced to make the return value extensible, it would be natural to add slice_pos to the dict in a follow-up. Not a regression — this was pre-existing.

2. calculate_kspacePP correctly forwards output_as_dict — The deprecated wrapper properly passes through the new parameter. ✓

3. ext_test_report.py migration is correct — Old unpacking k_traj_adc, _, t_excitation, _, t_adc = self.calculate_kspace() replaced with dict access. ✓

4. test_sequence.py comparisons with Approx — Switched from tuple to dict comparison. Works because pytest.approx supports dicts natively. ✓

Verdict

Safe to merge. The refactoring faithfully preserves all original logic, the new output_as_dict feature is backwards-compatible (defaults to False), and the transfer actually fixed a latent bug in the warning message. The only optional follow-up would be including slice_pos in the dict output to eventually match MATLAB's calculateKspacePP.

[schuenke]

Since I did a lot of refactoring myself, I would suggest that somebody else reviews this PR as well.

[mzaiss]

Since I did a lot of refactoring myself, I would suggest that somebody else reviews this PR as well.

I would be happy to review it.

[schuenke]

Since I did a lot of refactoring myself, I would suggest that somebody else reviews this PR as well.

I would be happy to review it.

Sounds good. Thanks.

[schuenke]

Before merging, I would also add slice_pos to the dict, but didn't want to push it yet because I wasn't sure if you already started reviewing this @mzaiss

[mzaiss]

please add the slice_pos parameter to the dict,
but also add a deprecation warning for dict=False, so we can proceed in breaking this in the next version.

I will review then after your updates.

[schuenke]

@mzaiss

• added slice_pos to dict and ensured its always a np.array
• added deprecation notice for output_as_dict=False (I decided to use logger.warning()for this because this will be visible in the terminal when executing code, but it does not break our own and related project's tests as a DeprecationWarning would do... )

We can still add a DeprecationWarning in the future, but we are currently collecting several breaking changes that we would like to introduce in a dedicated release.