Hacky fix for waveforms_and_times rf append floating point issue. #114
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.
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 thet[-1]+eps, whereepsis supposedly "smallest floating increment". These are all inline with upstream MATLABPulseqimplementation.Trouble begins with the
eps.epsis not actually the smallest increment. Floating point have variable increments depending on the number of significant digits. Quotingnumpydocumentation:It is only true if we are close to 1.0. Time vector is an unbounded vector, so our comparison with
epsbecomes incorrect as we addepsto larger and larger floats. When we applynp.unique()to these values, they are identified as duplicates, thus we raise:I could not find a solution to fix this without changing the way we look at things here, so I just added
50*epsarbitrarily 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.uniqueis 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 replacednp.uniquewith a call tonp.diffand a comparison witheps. This will make sure that time points are monotonically increasing. This way, I observed around 2.7 times improvement in speed.np.diffis still slow, in the near future, a better approach is needed.Sorry for the long post, let me know what you think.