Skip to content
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

Hacky fix for waveforms_and_times rf append floating point issue. #114

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

btasdelen
Copy link
Collaborator

When we have RF pulses with non-zero start/end times, waveforms_and_times() function tries to add trailing/leading zeros as necessary to the beginning and end of the RF waveform. To avoid having duplicate times, leading zero is added at time t[0]-eps, trailing zero is added to the t[-1]+eps, where eps is supposedly "smallest floating increment". These are all inline with upstream MATLAB Pulseq implementation.

Trouble begins with the eps. eps is not actually the smallest increment. Floating point have variable increments depending on the number of significant digits. Quoting numpy documentation:

eps: float

The difference between 1.0 and the next smallest representable float larger than 1.0. For example, for 64-bit binary floats in the IEEE-754 standard, eps = 2**-52, approximately 2.22e-16.

It is only true if we are close to 1.0. Time vector is an unbounded vector, so our comparison with eps becomes incorrect as we add eps to larger and larger floats. When we apply np.unique() to these values, they are identified as duplicates, thus we raise:

raise Warning(
                           "Not all elements of the generated time vector are unique."
                        )

I could not find a solution to fix this without changing the way we look at things here, so I just added 50*eps arbitrarily to silence the error. I am open to suggestions on how to solve this properly. I am also not sure why we need to add trailing or leading zeros, maybe the answer lies removing that.


The second issue this PR addresses is, using np.unique is extremely slow, and prohibits the use of the function for large sequences. It seems like it is used as a check to see if we have repeating time points. I would say it is an overkill. We do not expect to have an arbitrarily ordered time vector. If there is a repeat, it is most likely happening at consecutive time points. Thus, I replaced np.unique with a call to np.diff and a comparison with eps. This will make sure that time points are monotonically increasing. This way, I observed around 2.7 times improvement in speed. np.diff is still slow, in the near future, a better approach is needed.

Sorry for the long post, let me know what you think.

@btasdelen btasdelen requested a review from sravan953 April 26, 2023 23:26
@sravan953 sravan953 added the enhancement New feature or request label May 3, 2023
@sravan953 sravan953 self-assigned this May 3, 2023
pypulseq/Sequence/sequence.py Outdated Show resolved Hide resolved
pypulseq/Sequence/sequence.py Show resolved Hide resolved
@sravan953 sravan953 added the bug-fix Fixes something label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Fixes something enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants