Implement fromSetA for IntMap and Map

[L0neGamer]

Initial stab at #875

Happy to adjust, test, or benchmark as wanted. I'll look into benchmark comparisons for the changed functions later.

[treeowl]

Whoops ... I forgot that you'll need CPP around the coercions.

[treeowl]

I forgot to mention the need for QuickCheck properties. At the least, we need one that will verify that the effect order is correct. The simplest thing will probably be to work in Writer, recording each key as it's reached.

[treeowl]

Ideally, we'd want strictness tests too. I'm just waking up, so I haven't re-reviewed yet.

[L0neGamer]

No worries, not requiring a review just yet since I've not done benchmarks. I appreciate the comments, there's oh so much to make sure is done.

[L0neGamer]

Good call on the tests, the ordering of map's fromSetA was wrong (it was doing root, then left, then right, should've been left, root, right)

[treeowl]

What's this coercing from/to?

[L0neGamer]

looks like this is a confusing pattern in this and other testing files, where we coerce to and from Bot A instead of just using it. I'll remove it in this case, but there are 142 usages of coerce in just intmap-strictness alone so I won't change all of them for now.

[treeowl]

This all looks great, pending benchmarks. As long as fromSet doesn't regress, I think it should be good to go.

[L0neGamer]

Turns out pre-9.0.2 Solo does not exist, so I'll whip up an equivalent.

[treeowl]

Turns out pre-9.0.2 Solo does not exist, so I'll whip up an equivalent.

Ugh. And then there was a nontrivial amount of churn after its introduction with the (unfortunate, IMO) constructor name change. Feel free to roll your own unconditionally if you don't want to futz with all that CPP.

[treeowl]

Wait a sec ... That wasn't very smart of me.... In the test suite we can use whatever packages we like. So we can just depend on OneTuple and get Solo everywhere. I'm sorry for wasting your time.

[L0neGamer]

and of course liftA2 was not a method of Applicative at some point, so I will switch over to OneTuple as suggested.

No worries on wasting my time, sorry to waste yours.

[L0neGamer]

This time I tried to compile with ghc-9.2 so I know that this works with at least 9.2.2.

[treeowl]

I wish there were some way to tell GitHub to approve CI for a new contributor before their first PR is merged. Grrr.

[treeowl]

CPS writer is overkill here. We're just using Writer, not WriterT. It might be sensible to work in the Endo [a] monoid to reassociate concatenations.

[L0neGamer]

as you say, CPS writer is overkill. Unless you really want it, I'll switch to lazy writer and call it a day.

[treeowl]

That's fine. Our trees are balanced, so it shouldn't be a huge perf hit.

[L0neGamer]

I've added INLINABLE and as expected it really improves the benchmarks, nice to see things working

[L0neGamer]

map new vs old, intmap new vs old

[L0neGamer]

We don't currently have any benchmarks for traversal functions, so adding benchmarks for fromSetA and traversal functions that use transformers and such would be great in a followup PR.

[treeowl]

I would be more comfortable if I saw a few hundred milliseconds rather than microseconds. Is that feasible? Also, you're only using an IntSet consisting of a contiguous block of numbers. There's also a pseudo-random list in the test suite; could you possibly use that as well (or instead)?

[L0neGamer]

I can easily just bump the bound size up arbitrarily. I'm worried about bumping the benchmarks above what is reasonable for people to run normally. I'll change the sets used to be the random ones, as well.

[treeowl]

How long is it taking you to run the benchmarks?

[L0neGamer]

When I was running all of them it was taking a while, it takes less time when I'm just running these. Increaasing the test cases for fromSet and co to hundreds of milliseconds would be increasing the lengtth of the tests by three orders of magnitude, however. My computer is also relatively new so I'd worry for other's setups, or if we were to set these benchmarks up in future.

[treeowl]

Then don't bump them up too much in the PR, but do bump them up temporarily to produce some bugger numbers. We can deal with the larger question as a separate issue. Note that you can run specific benchmarks, or even just comment out everything else, to get the numbers you need.

[L0neGamer]

Here are some larger benchmarks with randomised sets.

[treeowl]

Nice results. We still need to work on some benchmarking issues, but this PR is not the place. Please remember also to update the change log.

[treeowl]

Thanks again for all your work!

[treeowl]

I'll merge once the final CI run completes, unless @meooow25 has any objections.

[meooow25]

Looks like I'm late, but leaving some comments anyway.

[meooow25]

These shouldn't be necessary.
In fact if we need them, we're doing something odd. There is no reason to generate functions that take in Bot a.

[treeowl]

I don't know why you say that. It avoids having to unwrap the Bots manually.

[L0neGamer]

I'm happy to remove this theoretically. Note that Fun from quickcheck cannot generate lazy functions, so it is a little bad to have this instance theoretically.

[meooow25]

It avoids having to unwrap the Bots manually.

Do you have any example of where we would want this instance?

[meooow25]

This is a weird way to do this.

What we want in the test here is a Key -> Solo A where A can be bottom. That translates to generating an arbitrary Func Key (Solo (Bot A)), which can be coerced to Key -> Solo A.

[L0neGamer]

Func Key (Bot A) and Func Key (Solo (Bot A)) are equivalent, and the only difference is whether you unwrap the Solo in the RHS or you wrap with Solo on the LHS. I preferred the former because it's clearer what the functions is meant to do - produce a bottom value from a key.

[meooow25]

It's not about what might seem clearer in this particular case, it's a simple mechanical translation from the type of the function being tested which can be applied to all tests here.

[treeowl]

@meooow25 I'm sorry I jumped the gun on the merge.

[L0neGamer]

I'll look into improvements in a follow up PR, and request your approval for that in time @ meooow25

[L0neGamer]

Func Key (Bot A) and Func Key (Solo (Bot A)) are equivalent, and the only difference is whether you unwrap the Solo in the RHS or you wrap with Solo on the LHS. I preferred the former because it's clearer what the functions is meant to do - produce a bottom value from a key.

[L0neGamer]

I'm happy to remove this theoretically. Note that Fun from quickcheck cannot generate lazy functions, so it is a little bad to have this instance theoretically.

[meooow25]

@meooow25 I'm sorry I jumped the gun on the merge.

No worries, these changes are fine. And of course, we can adjust things later if we need to.

I'll look into improvements in a follow up PR, and request your approval for that in time @ meooow25

Thank you, that would be great!

[meooow25]

It avoids having to unwrap the Bots manually.

Do you have any example of where we would want this instance?

[meooow25]

It's not about what might seem clearer in this particular case, it's a simple mechanical translation from the type of the function being tested which can be applied to all tests here.