Add `dropRight` and `takeRight` by markus1189 · Pull Request #191 · haskell/vector

markus1189 · 2017-12-18T17:59:36Z

I missed those functions recently and added them.

This was quite a bit of boilerplate 😞

OlivierSohn · 2018-01-07T17:59:01Z

I suggest a change in names : dropEnd, takeEnd, I find it is less ambiguous than dropRight, takeRight (this is also the naming used in textpackage : http://hackage.haskell.org/package/text-1.2.2.1/docs/Data-Text.html#v:dropEnd)

markus1189 · 2018-01-07T18:26:30Z

Sure, done!

OlivierSohn · 2018-01-07T19:03:11Z

Should the unsafe versions be added, too? I see that for example take and drop have unsafe versions.

basile-henry · 2018-01-07T19:06:30Z

+  where
+    n' = max n 0
+    m = length v
+    i = max 0 (m - n)


What happens if n is negative? What about doing max 0 (m - n') instead? 😄

OlivierSohn

Based on the code I read for dropand take functions, I think the use of delay_inline could be important to get the best possible optimizations. I might be wrong because I'm not familiar with fusion, but I think using the same approach as what in done in take and dropis a safe default.

EDIT : As I said in an earlier comment, it would be nice to have the unsafe versions too.

OlivierSohn · 2018-01-07T20:01:14Z

+-- contain less than @n@ elements in which case it is returned unchanged.
+takeEnd :: Vector v a => Int -> v a -> v a
+{-# INLINE_FUSED takeEnd #-}
+takeEnd n v = unsafeSlice i (min n' m) v


Based on the use of delay_inline in takefunction, I think this should be:

unsafeSlice i (delay_inline min n' m) v

OlivierSohn · 2018-01-07T20:03:07Z

+  where
+    n' = max n 0
+    m = length v
+    i = max 0 (m - n)


Based on the use of delay_inline in take function, I think this should be:

i = delay_inline max 0 (m-n')

OlivierSohn · 2018-01-07T20:03:57Z

+-- contain less than @n@ elements in which case an empty vector is returned.
+dropEnd :: Vector v a => Int -> v a -> v a
+{-# INLINE_FUSED dropEnd #-}
+dropEnd n v = unsafeSlice 0 (max 0 (m - n')) v


Here also, I think we need to add a delay_inline for the max

OlivierSohn · 2018-01-07T20:06:39Z

+  where
+    n' = max n 0
+    m = length v
+    i = max 0 (m - n)


I guess it's safer to write max 0 (m-n')

Agreed. Done

OlivierSohn · 2018-01-07T20:11:20Z

  model = V.toList
  unmodel = V.fromList

+naiveTakeEnd :: Int -> [a] -> [a]


This test function is duplicated to be used in two files, I would rather put it in a separate module to avoid duplication, like the existing Utilitiesmodule.

Moved to Utilities

OlivierSohn · 2018-01-07T20:11:31Z

+naiveTakeEnd :: Int -> [a] -> [a]
+naiveTakeEnd n = reverse . take n . reverse
+
+naiveDropEnd :: Int -> [a] -> [a]


This one, too.

Moved to Utilities

markus1189 · 2018-01-10T14:43:36Z

Thanks for the thorough review! Next step is to add the unsafe versions as well then. I wish it would be less boilerplate-y though

OlivierSohn · 2018-01-10T17:21:42Z

@markus1189 Yes, it looks like it will be a lot of copy / paste / change one word stuff :)

Maybe you can ask a maintainer of the library if it's necessary or not, before doing the work of adding the unsafe versions. (I'm not a maintainer myself)

markus1189 · 2018-01-11T13:35:38Z

I added the unsafe versions. I hope I did not make any copy & paste errors ;)

OlivierSohn · 2018-01-11T15:43:46Z


+unsafeTakeEnd :: MVector v a => Int -> v s a -> v s a
+{-# INLINE unsafeTakeEnd #-}
+unsafeTakeEnd n v = unsafeSlice (m - n) m v


the second argument is wrong I think, it should be n ?

Maybe adding tests for the unsafe versions would be a good idea (the same tests as for the safe ones)

OlivierSohn · 2018-01-11T15:48:57Z

+-- contain at least @n@ elements but this is not checked.
+unsafeTakeEnd :: Vector v a => Int -> v a -> v a
+{-# INLINE unsafeTakeEnd #-}
+unsafeTakeEnd n v = unsafeSlice (m - n) m v


Here too, m should be n

markus1189 · 2018-01-17T19:35:01Z

I am willing to continue to work on this, but before I do I would like to have some feedback from the maintainers whether this is wanted at all. 😉

cartazio · 2018-01-17T20:29:53Z

Probably? I / Dan / Ryan are eyeballs deep in work / oss backlog (or at least I am :)) Hit me up this Saturday morning/ midday on freenode on one of the Haskell channels (#numerical-haskell may have some folks who can also help review ) if I’ve not looked at it by then. Or Twitter. Handles there are respectively Carter and cartazio. Cheers.

…

On Wed, Jan 17, 2018 at 2:35 PM Markus Hauck ***@***.***> wrote: I am willing to continue to work on this, but before I do I would like to have some feedback from the maintainers whether this is wanted at all. 😉 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#191 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwlqHevG5Hx4seEgYjybb70G7XYMIks5tLktmgaJpZM4RF2gw> .

markus1189 · 2018-01-18T10:12:18Z

Okay I'll try to fix the issues until Saturday. I don't know yet how much time I'll have on Saturday, but for the records: what's your timezone? :D I'm in CET (Frankfurt, Germany) On Wed, Jan 17, 2018 at 9:29 PM, Carter Tazio Schonwald < notifications@github.com> wrote:

…

Probably? I / Dan / Ryan are eyeballs deep in work / oss backlog (or at least I am :)) Hit me up this Saturday morning/ midday on freenode on one of the Haskell channels (#numerical-haskell may have some folks who can also help review ) if I’ve not looked at it by then. Or Twitter. Handles there are respectively Carter and cartazio. Cheers. On Wed, Jan 17, 2018 at 2:35 PM Markus Hauck ***@***.***> wrote: > I am willing to continue to work on this, but before I do I would like to > have some feedback from the maintainers whether this is wanted at all. 😉 > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#191 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ AAAQwlqHevG5Hx4seEgYjybb70G7XYMIks5tLktmgaJpZM4RF2gw> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#191 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkGz1vrf6M_nl596eiok4frB6iJjFt6ks5tLlhCgaJpZM4RF2gw> .

cartazio · 2018-01-18T21:19:26Z

NYC time :) On Thu, Jan 18, 2018 at 5:12 AM, Markus Hauck <notifications@github.com> wrote:

…

Okay I'll try to fix the issues until Saturday. I don't know yet how much time I'll have on Saturday, but for the records: what's your timezone? :D I'm in CET (Frankfurt, Germany) On Wed, Jan 17, 2018 at 9:29 PM, Carter Tazio Schonwald < ***@***.***> wrote: > Probably? > I / Dan / Ryan are eyeballs deep in work / oss backlog (or at least I am > :)) > > Hit me up this Saturday morning/ midday on freenode on one of the Haskell > channels (#numerical-haskell may have some folks who can also help review ) > if I’ve not looked at it by then. Or Twitter. Handles there are > respectively Carter and cartazio. > > Cheers. > > > > On Wed, Jan 17, 2018 at 2:35 PM Markus Hauck ***@***.***> > wrote: > > > I am willing to continue to work on this, but before I do I would like to > > have some feedback from the maintainers whether this is wanted at all. 😉 > > > > — > > You are receiving this because you are subscribed to this thread. > > Reply to this email directly, view it on GitHub > > <#191 (comment)>, or > mute > > the thread > > <https://github.com/notifications/unsubscribe-auth/ > AAAQwlqHevG5Hx4seEgYjybb70G7XYMIks5tLktmgaJpZM4RF2gw> > > . > > > > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#191 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAkGz1vrf6M_ nl596eiok4frB6iJjFt6ks5tLlhCgaJpZM4RF2gw> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#191 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwrWPOaCUJHxZamahWHwUFwlzqauPks5tLxkCgaJpZM4RF2gw> .

markus1189 · 2018-01-19T14:46:14Z

I kind of gave up on writing the prop for the naive version. It seems like there is quite some customizing going on in the QuickCheck tests.

Some help would be great there ;)

@OlivierSohn Fixed the issues you raised, thanks a lot for the in-depth reviews!

cartazio · 2018-01-21T18:31:07Z

possibly dumb question, and this might just be me missing a difference ... but how is takeEnd different from drop?, just a difference in in counting from front of vector vs end of vector?

this seems to be a useful difference, and the key one that motivates the changes?

cartazio · 2018-01-21T18:36:40Z

so one possible question about this for code simplification: is there any reason to not define takeEnd in terms of drop, and dropEnd in terms of take? (one reason not to would be if the generated core was less awesome under O2 or something)

cartazio · 2018-01-21T18:38:06Z

also you duplicated an Orphan, https://travis-ci.org/haskell/vector/jobs/330841124#L908-L918
use the orphan module for that

cartazio · 2018-01-21T18:38:43Z

 -- TODO: test non-IVector stuff?

+#if !MIN_VERSION_base(4,7,0)
+instance Foldable ((,) a) where


ORPHAN, see Data.Orphan in tests

markus1189 · 2018-01-21T18:54:26Z

@cartazio Yes indeed, takeEnd is drop but avoids the calculation of correct offsets (equivalently for take and dropEnd)

I missed those handy functions recently and thought to add them (this was in 24pullrequests btw :) )

This got a little out of hand, I thought it would be much less stuff to do, though 😉

Will check the orphans

cartazio · 2018-01-21T19:07:59Z

I guess my point is that if those overheads/extra calculations are optimized away under O2 level optimization and have equivalent fusion characteristics, may as well do the simpler definition in terms of preexisting high level operations. I would be surprised if the optimized core isn't essentially the same. (worth a bug report even :) )

OlivierSohn · 2018-01-21T20:44:22Z

also you duplicated an Orphan, https://travis-ci.org/haskell/vector/jobs/330841124#L908-L918
use the orphan module for that

I see the same error on my pull request #196, strange... is it possible to launch a build of master to see if the build is still ok?

cartazio · 2018-01-21T21:11:58Z

https://travis-ci.org/haskell/vector/builds/331565078

cartazio · 2018-01-21T21:40:41Z

master is fine, this patch is the issue :)

cartazio · 2018-01-21T21:41:33Z

i restarted the PR build so we can look again

markus1189 · 2018-02-08T07:16:00Z

Sorry but I have to abandon this as I don't come around to do it. If anyone wants to pick it up, feel free ;)

noughtmare · 2022-12-21T13:31:28Z

I think it would bo good to consider using the names dropL, dropR, takeL, and takeR (like what the Data.Sequence module in the containers library does). Because vectors have no inherent bias to left or right (except perhaps the fusion mechanism?). Text (and arguably bytestrings) do have such a bias, so the asymmetric naming scheme makes sense for those types.

lehins · 2022-12-22T19:22:14Z

@noughtmare Would you like to create an issue with a link to this PR, so it doesn't get forgotten and someone with time on their hands can pick it up.

markus1189 force-pushed the drop-take-right branch 2 times, most recently from 62b3b1b to 35277d8 Compare January 7, 2018 18:26

basile-henry reviewed Jan 7, 2018

View reviewed changes

OlivierSohn suggested changes Jan 7, 2018

View reviewed changes

markus1189 force-pushed the drop-take-right branch from 3591ce1 to 8fc97c6 Compare January 10, 2018 14:45

markus1189 force-pushed the drop-take-right branch 2 times, most recently from 3b5c20c to 8b05a50 Compare January 11, 2018 07:49

OlivierSohn reviewed Jan 11, 2018

View reviewed changes

markus1189 force-pushed the drop-take-right branch 2 times, most recently from 153bb8e to 90dd307 Compare January 19, 2018 14:44

cartazio reviewed Jan 21, 2018

View reviewed changes

Add dropEnd and takeEnd

7ed68e5

markus1189 force-pushed the drop-take-right branch from 90dd307 to 7ed68e5 Compare January 21, 2018 18:54

markus1189 closed this Feb 8, 2018

noughtmare mentioned this pull request Dec 21, 2022

Add takeEnd to Data.Sequence haskell/containers#884

Closed

Conversation

markus1189 commented Dec 18, 2017

Uh oh!

OlivierSohn commented Jan 7, 2018

Uh oh!

markus1189 commented Jan 7, 2018

Uh oh!

OlivierSohn commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlivierSohn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markus1189 commented Jan 10, 2018

Uh oh!

OlivierSohn commented Jan 10, 2018

Uh oh!

markus1189 commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markus1189 commented Jan 17, 2018

Uh oh!

cartazio commented Jan 17, 2018 via email

Uh oh!

markus1189 commented Jan 18, 2018 via email

Uh oh!

cartazio commented Jan 18, 2018 via email

Uh oh!

markus1189 commented Jan 19, 2018

Uh oh!

cartazio commented Jan 21, 2018

Uh oh!

cartazio commented Jan 21, 2018

Uh oh!

cartazio commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markus1189 commented Jan 21, 2018

Uh oh!

cartazio commented Jan 21, 2018

Uh oh!

OlivierSohn commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cartazio commented Jan 21, 2018

Uh oh!

cartazio commented Jan 21, 2018

Uh oh!

OlivierSohn left a comment •

edited

Loading

OlivierSohn commented Jan 21, 2018 •

edited

Loading

noughtmare commented Dec 21, 2022 •

edited

Loading