Conversation
a5c4a0b to
1060462
Compare
|
@treeowl Could you say something about the motivation for this PR? Ideally create an issue, so we can discuss the "problem" separately from this implementation. |
| uncheckedSplit :: Sized a => Int -> FingerTree a -> UncheckedSplit a | ||
| uncheckedSplit i ft | ||
| | S.Split l m r <- S.splitTree i ft | ||
| = UncheckedSplit l m r |
There was a problem hiding this comment.
Instead of this partial function, why not only export split and let users deal with partial patterns if they want?
There was a problem hiding this comment.
My only concern is whether that extra stuff will inline away. Certain functions are extremely sensitive to the performance of splitting tiny little sequences (e.g., 2–5 elements) where any little extra time/allocation can matter a lot. I'm not sure how hard it would be to rejigger Data.Sequence.Internal.splitTree to make sure that works out okay.
1060462 to
5e746ca
Compare
What really triggered it for me was wanting to play with incremental quicksort for sequences. Doing that (reasonably) efficiently requires use of In principle, it would be nice to expand from data FingerTree s a
class Sized s a | a -> s where size :: a -> s
(<|) :: (Coercible s Int, Monoid s, Sized s a) => a -> FingerTree s a -> FingerTree s abut then we're relying on an additional specialization along with all that non-portable stuff. Youch. So I figured I'd start with something conservative. |
5e746ca to
035e634
Compare
* Export a type for `(Int,+)` finger trees. * Export more `Data.Sequence` internals. * Offer a module of `Data.Sequence` internals intended for external use, that should obey the PVP.
|
I was about to mention https://hackage.haskell.org/package/fingertree but if the goal is to implement "incremental quicksort" for |
Well... not strictly necessary, I don't think, but the |
|
So, do you intend to offer this incremental quicksort from a different package? Otherwise I don't understand why we need to enhance the public / stable API for this. Or are you possibly talking about different sequence types than |
|
I don't even know if I'll be able to make that practical; it's just what
got me here. I think another application is ropes: finger trees of byte
arrays.
…On Wed, Feb 3, 2021, 6:41 PM Simon Jakobi ***@***.***> wrote:
So, do you intend to offer this incremental quicksort from a different
package? Otherwise I don't understand why we need to enhance the public /
stable API for this.
Or are you possibly talking about different sequence types than Seq here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOOF7M4NJQRRIZPDL4CEETS5HNKTANCNFSM4XBTXOVA>
.
|
emilypi
left a comment
There was a problem hiding this comment.
This looks good on first pass!
Unless i'm missing something (i'm not super familiar with this library's internals), there are a few improvements we can make to ergnomics here. For example, I cannot fromList [1..10]. I must fromList $ Elem <$> [1..10], since Sized has only two instances. The ability to debug splits would also be welcome.
Thoughts?
|
|
||
| data Split a | ||
| = Split !(FingerTree a) a !(FingerTree a) | ||
| | EmptySplit |
There was a problem hiding this comment.
Do we have a debugging function that shows the split lying around? Would be useful to have.
That |
phadej
left a comment
There was a problem hiding this comment.
I cannot comment on this (not sure why you asked me to review). finger trees are unfamiliar structures to me.
| anything internal that is not exported, please file a GitHub issue. | ||
| -} | ||
|
|
||
| module Data.Sequence.StableInternal |
There was a problem hiding this comment.
Would Data.Sequence.Unsafe be better name?
(Int,+)finger trees.Data.Sequenceinternals.Data.Sequenceinternals intendedfor external use, that should obey the PVP.
FunctorandTraversableinstances forFingerTreeandNode.Generic1instance forFingerTree.