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

Add ShiftedArrays.diff for differences between elements in a vector #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pdeffebach
Copy link

Base.diff does not return a vector of the same size as the original. I find this very frustrating, as I express in #41.

This PR adds ShiftedArrays.diff as a function to get the difference between elements, but pre-pends missing at the first element.

I understand that ShiftedArrays.diff is more characters than x .- lag(x), but it's more declarative.

@nalimilan
Copy link
Contributor

I agree we really need this, but it's not specific to ShiftedArrays at all since it returns a plain Array. Maybe we could add an argument like default to diff in Base to enable this behavior? At least it's probably worth trying.

@pdeffebach
Copy link
Author

Filed an issue here

@piever
Copy link
Collaborator

piever commented Oct 5, 2021

I also think it's a bit odd that this should live here. IMO, the idea behind ShiftArrays.circshift versus Base.circshift (or fftshift and co.) is to return a lazy object with the same content. It's the same logic underlying Iterators.map versus Base.map.

Returning an eager result whose value differs from Base.diff is a bit odd, it would be more logical to add options to diff. That being said, I agree that the functionality is useful, I'm just unsure it should be called ShiftedArrays.diff.

@nalimilan
Copy link
Contributor

Ah yeah so you mean it would make sense to have ShiftedArrays.diff return a lazy array instead, with arguments to choose whether the first value should be missing or not?

@pdeffebach
Copy link
Author

If it's performant to have the object be lazy, and if it takes advantage of the same infrastructure as lag, then it should definitely live here, right?

@piever do you want to experiment with making this be lazy? Or should I modify the PR to make it return something which looks like what lag returns.

@piever
Copy link
Collaborator

piever commented Oct 5, 2021

Ah yeah so you mean it would make sense to have ShiftedArrays.diff return a lazy array instead, with arguments to choose whether the first value should be missing or not?

I think it'd make sense for it to return a lazy object, as for instance

diff(v, n = 1; default) = Broadcast.broadcasted(-, v, lag(v, n; default))

My main concern was that collect(ShiftedArrays.diff(v)) != diff(v), whereas for all the other functions shadowed here (circshift, fftshit, ifftshift) that holds. Maybe that's not so bad with the mandatory keyword argument. I imagine if diff(v, default=missing) were to be added to Base, it would have the same meaning as the definition above but eager.

@pdeffebach
Copy link
Author

Okay, I see your point about consistency with Base. Sorry for missing it earlier. If possible can you show your support in the linked issue? Would be good to coalesce these two discussions into one.

@@ -4,6 +4,7 @@ import Base: checkbounds, getindex, setindex!, parent, size
export ShiftedArray, ShiftedVector, shifts, default
export CircShiftedArray, CircShiftedVector
export lag, lead
# ShiftedArrays.diff unexported due to collision
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should not be exported - no need to comment I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants