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

Remove pull array index (unsafe), add uncons. #475

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

sjoerdvisscher
Copy link
Member

@sjoerdvisscher sjoerdvisscher commented Apr 4, 2024

Fixes #471

I removed index, but it was used by the example in Data.Array.Polarized, so I added uncons which seems to be what the example actually needed, and it's clear that it's safe.

@b-mehta
Copy link
Contributor

b-mehta commented Apr 4, 2024

I agree uncons is safe and is what's actually needed in the example, but in the Nothing case loop should return an empty array, I'm pretty sure.

@sjoerdvisscher
Copy link
Member Author

sjoerdvisscher commented Apr 5, 2024

I agree uncons is safe and is what's actually needed in the example, but in the Nothing case loop should return an empty array, I'm pretty sure.

That's what it is doing. The error is a filler for a function that should never be called. I kept it from the original example, and my first reaction was the same as yours, so perhaps we should fix it. I guess adding an empty function would clarify things.

Comment on lines +117 to +120
-- | Decompose an array into its head and tail, returns @Nothing@ if the array is empty.
uncons :: Array a %1 -> Maybe (a, Array a)
uncons (Array _ 0) = Nothing
uncons (Array f n) = Just (f 0, fromFunction (\x -> f (x + 1)) (n - 1))
Copy link
Member

Choose a reason for hiding this comment

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

Thought: a natural generalisation of uncons is splitAt, with

splitAt :: Int -> Array a %1 -> Maybe (Array a, a, Array a)
splitAt i (Array f n)
  | n <= i = Nothing
  | otherwise = Just (fromFunction f i, f i, fromFunction (\x -> f (x+i+1)) (n-i-1)

With uncons is (up to isomorphism) splitAt 0.

Do you think it's worth exposing this generalisation?

Copy link
Member Author

Choose a reason for hiding this comment

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

When implementing uncons with splitAt 0, is there a way to discard the initial array in a total way? It should be empty, but there's no proof.

Copy link
Member

Choose a reason for hiding this comment

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

We don't appear to have made such a function available. But we should. Of course, it'd be partial and be an error if the length is non-0.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe splitAt isn't a great name. At least we've used split for another similar function which doesn't have the singled-out element. (this means we can also define splitA, whatever we name it, as a composition of split and uncons).


Ok, so, now that I think about. There is a stupid way to disappear the null array: let !(a1, x, a2) in (x, append a1 a2). Is it a good idea? Probably not. But it's possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point about splitAt being the composition of split and uncons. It feels like that splitAt doesn't add much then?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe not. Let's merge this PR at any rate.

You know what's funny though? splitAt = traverse uncons split may be the first time I see the Traversable (a,) instance show up in practice.

@sjoerdvisscher sjoerdvisscher merged commit 16795c7 into master Apr 9, 2024
9 checks passed
@sjoerdvisscher sjoerdvisscher deleted the sv/remove-pull-array-index branch April 9, 2024 08:13
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.

Pull array index isn't safe
3 participants