-
Notifications
You must be signed in to change notification settings - Fork 64
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
Reworked check_timing
to provide a structured error report
#200
base: dev
Are you sure you want to change the base?
Conversation
(This should be caught by `make_adc`. If we get here, the user really wants to adjust the delay and just accept it and let `check_timing` report it)
- Added unit test for `check_timing` - The default signature of `seq.check_timing` remains the same, but can pretty-print the report with `print_errors=True` - `seq.write` now checks sequence timing by default - 'TotalDuration' calculation moved from `check_timing` to `seq.write` - Check whether gradients in the last block ramp down to zero removed from `check_timing`
- Can be enabled/disabled with `pp.enable_trace(limit)` and `pp.disable_trace()` - Added tracing to all blocks and all RF/ADC/gradient events - Added printing of the trace to `check_timing` when it is available
…e `TotalDuration` in the sequence definitions
|
Also there is a trivial conflict occured due to merging of the other PR. |
|
In that case, adding it to |
- `Sequence.write` now checks whether the last block ramps down to 0 - Fixed gradient continuity checks in `set_block` - Now handles arbitrary block orders properly - Now properly checks the last value of the previous block - Added gradient checks for blocks without gradients - Sequence object now remains valid after gradient errors - Added unit tests for gradient continuity in `test_block`
I added the gradient check to A note for some future improvements: With the tracing information, runtime errors in I'll fix the merge conflict once we're happy with the PR as a whole. |
fix some type hints some cleanup
I finally tested your changes and like it a lot. I (and my linter) did some further cleanups and I also adjusted the test_report() function to print the timing errors in a bit more structured way. It will print the total number of timing errors and then list up to 20 errors. The timing check report is now at the bottom of the report and each (of the up to 20) error will now be on a new line for better readability. Looks for example like this:
I also realized that we use a mixture of single quotes and double quotes in all our code, with less appearance of single quotes compared to double quotes. This is why I changed all single quotes to double quotes in the files I touched (although I personally prefer single quotes). I would suggest to introduce RUFF as a linter and formatter for PyPulseq. We can use a very basic config in the beginning that just checks for correct order of imports, single/double quotes, white spaces, line endings etc. I will create another PR for this. let me know if you want to review this again @FrankZijlstra & @btasdelen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. See my previous comment for more details.
Changes look good to me. And agreed on the formatting, I also prefer single quotes (hence all the single quotes in my code), but most importantly it should be consistent. |
As per title:
check_timing
now provides a list ofSimpleNamespace
error descriptions, e.g.namespace(block=2, event='block', field='duration', error_type='BLOCK_DURATION_MISMATCH', value=0.00102, duration=0.001)
check_timing
remains the same to not break existing code. It will now just print the namespace objects instead the error messages.print_error_report
function provide incheck_timing.py
.check_timing
now accepts an additional argumentprint_errors=True
to directly print the error report if there are errors.Sequence.write
now performs acheck_timing
check by default (can be disabled withcheck_timing=False
) and warns if any errors are found.Sequence.write
check_timing
check_timing
to not assumerf_raster_time == adc_raster_time
(Sub-microsecond ADC delays are dropped without notification when exporting to .seq file #191).In addition, the final commit in this series adds functionality to trace where sequence blocks and events were first created, which I believe helps a lot with identifying where the timing errors originate:
pp.enable_trace(depth=1)
andpp.disable_trace()
. It is disabled by default.check_timing
error report will automatically include them.event.trace
field if tracing is enabled. This is the call stack (up to the specified depth) from where the event was created.Some smaller changes included in this PR:
system == None
checks tosystem is None
and added proper optional type hints for system (Union[Opts, None]
).Queries:
print_error_report
by providingcolored=False
. But I wonder what others think, is this desired, and if so, are the colours nice, does it work in all terminals (or at least not break anything), etc.? Try:check_timing
there was a check whether gradients ramped down to zero in the last block. I removed this check because it does not belong in a timing check. Question is, where should it go? Also, it literally only checked the last block, all other gradient checks occur inadd_block
.make_*
functions.