add scanrPar, scanlPar

[vmchale]

This adds a low-depth scan, see Conal Elliott's paper for more. This uses a different approach, however - Haskell's "dependent types" rather than type families.

I added a new module because .hs-boot files don't allow pattern synonyms. The alternative would be to have two parts of Clash.Sized.Vector to avoid mutual module dependencies, which would be a larger diff.

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[DigitalBrains1]

Thanks for the nice piece of engineering!

I think it would be better if these functions were added to Clash.Sized.RTree (based on RTrees then, not Vecs) instead of creating a new small module specially for them. Is this possible?

[martijnbastiaan]

I appreciate the effort to include the SVG, but to include a proprietary, paid-for, macOS only application file in a BSD2 project is not the way to go IMO.

The diagram doesn't look to complicated, perhaps you could draw it in draw.io? Most of use VSCode nowadays, which would enable everyone to just edit it in their IDEs. It also exports to PNG/SVG.

[DigitalBrains1]

Martijn, did you see that there are already 31 other .graffle files in clash-prelude/doc? I've never looked at it before but it seems we already do this for 8 years, starting with 0824268.

[martijnbastiaan]

Oh wow. I didn't know.

[martijnbastiaan]

I stand by my comment but if that's what we have there's no problem merging this PR.

[alex-mckenna]

On the whole looks good. Thinking out loud, I wonder if we might want to also expose scanlInductiveRTree and scanrInductiveRTree as scans on trees (since you can really scan anything that's foldable if you believe). If you think that's a good idea they should probably be renamed tscanl and tscanr to better fit with the naming convention of other RTree functions

[martijnbastiaan]

I think you still need to update Clash.Prelude and Clash.Explicit.Prelude to have them exported there right?

[DigitalBrains1]

I think you still need to update Clash.Prelude and Clash.Explicit.Prelude to have them exported there right?

No, they both export RTree fully.

Furthermore, could you change the type to

scanlPar :: (a -> a -> a) -> RTree n a -> RTree n a

? I suppose that overlaps with Alex's suggestion and they would be better named tscanl1 and tscanr1. Those seem better than tscanl / tscanr to me. Incidentally, this might also fix your head and last issue perhaps?

I'm also not satisfied with the documentation yet. I appreciate the work that went into the nice picture (though I would have named op f to match the existing graphics), and it's a great addition, but the text still needs improvements, IMO. I can put some work into this myself tomorrow.

[DigitalBrains1]

I see we don't have head and last for RTree. It seemed such an obvious feature I hadn't checked. Maybe add those? :-)

[vmchale]

I don't think it's that productive to generalize tscanl and tscanr - they work for vectors quite sensibly but not sure about Foldable in general.

(I still need documentation for tscanl and tscanr).

[DigitalBrains1]

Are you saying you don't want

tscanl1 :: KnowNat d => (a -> a -> a) -> RTree d a -> RTree d a

or that you don't want

tscanl1 :: Traversable t => (a -> a -> a) -> t a -> t a

?

The former seems to make a lot of sense to me. The function is expressed in RTree, you're just doing v2t on the input. And since it is expressed in RTree, I don't see a reason to change the shape to Vec on the output, it can just output an RTree and if people would like to have it on Vec they can use t2v on that result. But they might have liked to keep the RTree shape instead of being forced to reconstruct it if it reshapes to Vec.

[alex-mckenna]

FWIW, when I mentioned Foldable it was in reference to "you can scan over any foldable thing to produce a Vec" (the part in italics I was maybe too implicit about). Producing an RTree instead like @DigitalBrains1 suggested and exposing you have now is as generalised as I was aiming for, I just wanted the scan{l,r} on RTree to be exported too.

Unrelated: I see a merge from this branch into itself in the commit history. Since we want to keep exposing the GADT constructors for RTree in a separate commit you should go back and tidy up the history here

[vmchale]

I think squashing everything is the way to go.

[vmchale]

@DigitalBrains1 yes! I expose tscanr, tscanl now. It's a little easier in some ways.

[DigitalBrains1]

A squash is fine by me. [edit] But I don't mean to say I disagree with Alex regarding separate commits, just that I don't mind either way! :-) [/edit]

I think it's unnecessary to also have scanlPar, people can do the v2t, t2v dance. I also prefer the name tscanl1 / tscanr1 myself since they are tree versions of scanl1 and scanr1.

[alex-mckenna]

I would prefer two commits since exposing the GADT constructors to the world is an unrelated change, but I don't have particularly strong feelings about it

[vmchale]

I think tscanr and tscanl are fine since there's so need to differentiate tscanr1 for RTrees - we only have tfold and that's fine!

[martijnbastiaan]

Agreed with @DigitalBrains1 on the documentation, otherwise LGTM!

[DigitalBrains1]

Should we change the name of the GADT constructors? It's kinda odd to export things named X_. I know we're trying to move away from primes, but maybe LR' and BR' are good names still. I'm not too enthousiastic about LR0 and BR0. LR# and BR# are also options. Although that's taking the magic hash rather far, naming your constructors for boxed values that. Let's bikeshed some more!

[vmchale]

I've used RBranch and RLeaf in case people already have their own stuff (since this is in Prelude)

[DigitalBrains1]

What is the reason for retaining scanlPar and scanrPar when we have tscanl and tscanr? We don't provide Vec wrappers for the other functions in Clash.Sized.RTree. Why not have people use v2t and t2v themselves?

[vmchale]

@DigitalBrains1 I could remove them I suppose - but they are what is most sensibly useful! Since vectors naturally lend themselves to an idea of scanning.

[DigitalBrains1]

You know, it occurred to me it is not a reasonable argument, because next to tfold we do actually have fold and tdfold cannot easily have a Vec counterpart. So it is not a break of pattern, there simply was no established pattern yet. Your honor, objection withdrawn!

[edit] The counterpart to tdfold is of course dtfold. * Puts on dunce's hat and goes to stand in the corner * [/edit]

[DigitalBrains1]

I've pushed additional commits on the mass of commits here already, but I've also prepared a version that I think is ready to be committed with a merge commit (don't squash or rebase!) here: parallel-scan-peter. You could force-push that over this branch if you want. I'm not force-pushing here without discussing.

I'd like it if someone reviewed my documentation (full changes here). I kinda feel like I had more text, but my dull head can't locate it in there.

[vmchale]

Thanks! I think if documentation is up to your standard, it should be merged then :)

[DigitalBrains1]

I'd like either @martijnbastiaan or @alex-mckenna to review my proposed documentation. If they think it's up to snuff, we can merge (again, a merge commit, not a squash or rebase. Really, GitHub should allow you to specify what type of merge you want and then warn the committer if they are about to go against that, to prevent accidents. We don't have that, so I'll just keep repeating it 🙂).

[martijnbastiaan]

Great docs. Thanks @DigitalBrains1, and thanks @vmchale for the implementations :).

[DigitalBrains1]

Actually, there was a real deficiency in the docs that I wrote with a rather dull brain. Before this sentence:

Circuit size is \(\mathcal{O}(n \cdot \log n)\), but logic depth is \(\mathcal{O}(\log n)\).

I added

When \(n\) is the number of elements, [...]

as this is not obvious at all, I could have meant tree depth, but I didn't.

While I needed to amend it anyway, I also changed "Extract the first element of an RTree" to "Extract the first element of a tree" to be more in line with the rest, same for tlast.