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.
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
ENH: Phase unwrapping generalized to arbitrary interval size #16987
ENH: Phase unwrapping generalized to arbitrary interval size #16987
Changes from 26 commits
29c0bb3
f080593
dfee885
6131310
0b7ad2c
2c0d4b8
24afdab
16e3bec
de7aa73
11ae434
8cf0872
c3ea9b6
3da0506
00dcda2
8755c7f
fde3fdb
7c56c83
a84b28a
d31cc73
dbb3472
e6bea5f
ba6fc3a
985c2a6
d7322c7
e54a06c
fa7f79d
f80fe62
f4c2d09
5bc6926
8bdeaeb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Isn't this diff allowed to be
= 255
? In general I'd be wary of tests usingrand
, because a badly written test causes CI failures at an unknown point in the future, rather than immediately.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.
This is a test I copy-and-pasted from the old PR and just adopted it to the new parameter names. I have to say, I don't really like it. Let's change that maybe.
My understanding was that this test should ensure that the correction is not too large, e.g. if a correction of 4 pi is needed, there should not be a correction of 6 pi, or so. But not so sure.
Regarding your question of the diff being
=255
, I think, that should not be the case. Let's say the period is2 pi
, after unwrapping there must not be a jump larger thanpi
. So jumps of2 pi
are not allowed.