Skip to content

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

Merged
sjoerdvisscher merged 2 commits intomasterfrom
sv/remove-pull-array-index
Apr 9, 2024
Merged

Remove pull array index (unsafe), add uncons.#475
sjoerdvisscher merged 2 commits intomasterfrom
sv/remove-pull-array-index

Conversation

@sjoerdvisscher
Copy link
Copy Markdown
Contributor

@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.

@sjoerdvisscher sjoerdvisscher requested a review from aspiwack April 4, 2024 16:05
@b-mehta
Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
@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