Added functionality to remove duplicate events #178
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added `remove_duplicates` in `event_lib` - Added `remove_duplicates` in `Sequence` - Changed `write_seq` to call `remove_duplicates` before writing - Changed `read_seq` to call `remove_duplicates` after reading
…it contains numpy data
… library is empty
…urn 0 instead of nan outside the specified gradients
schuenke
approved these changes
Jun 7, 2024
There was a problem hiding this comment.
I tested the Code with the problematic sequence from #169 and everyhing works fine. I didn't check all tests in detail, but the idea totally makes sense to me.
My comments are not meant as necessary changes, but only as a suggestion for better readability/debugging.
- Added two sequence tests including labels - Added check in sequence tests to test that the labels are identical after writing/reading
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR provides a basic fix for issue #169. Both
SequenceandEventLibrarynow have aremove_duplicatesfunction that removes events that would be identical after rounding. The function is called both beforeSequence.writeand afterSequence.readto ensure that no actual duplicate events exist in the event libraries.I opted not to do any rounding while adding events, because this affects all sequence analysis functions (e.g.
calculate_kspace), and it affects any use ofget_blockto copy events (i.e. there can be compounding rounding errors if events are stored rounded). The proposed solution is also a lot faster.This does mean that in the example of #169, the library will still contain a lot of events, which will only be filtered when writing the sequence. Alternatively,
seq.remove_duplicates(in_place=True)could be called manually at any point during the creation of a sequence to force rounding of the event libraries.In addition, I added a basis for
Sequenceunittests, testing whether sequences can be write and read back in, and recreated byadd_block(get_block(...)), and still produce (within rounding errors) the same sequence (tested withget_block,get_gradients, andcalculate_kspace). It also checks the .seq output compared to a known baseline, contained in thetests/expected_outputdirectory. The expected .seq output files can be regenerated with the current repository by setting a SAVE_EXPECTED environment variable before running pytest (e.g. if a change in the output happens that is confirmed to be correct).These tests should be expanded with further sequences snippets which tests all pypulseq functionality, and e.g. including all current example sequences.
Adding these tests uncovered two small bugs fixes in
Sequencethat I included here.