implement partition via partition-values

[benknoble]

Summary of Changes

• Implement partition via partition-values as in Generalized Sieve #40.
• Update docs & tests.

I didn't test this locally beyond what I had in my original local implementation, so I don't know if my code yet fits into the expected repo structures correctly. I'm hoping the GH action will tell :)

Public Domain Dedication

• In contributing, I relinquish any copyright claims on my contribution and freely release it into the public domain in the simple hope that it will provide value.

I'm leaving the below unless it's noise for anyone that isn't me and can be removed.

(Why: The freely released, copyright-free work in this repository represents an investment in a better way of doing things called attribution-based economics. Attribution-based economics is based on the simple idea that we gain more by giving more, not by holding on to things that, truly, we could only create because we, in our turn, received from others. As it turns out, an economic system based on attribution -- where those who give more are more empowered -- is significantly more efficient than capitalism while also being stable and fair (unlike capitalism, on both counts), giving it transformative power to elevate the human condition and address the problems that face us today along with a host of others that have been intractable since the beginning. You can help make this a reality by releasing your work in the same way -- freely into the public domain in the simple hope of providing value. Learn more about attribution-based economics at drym.org, tell your friends, do your part.)

[benknoble]

@countvajhula I am now remembering some things about Racket BC that may make implementing this via qi/macro harder. Any thoughts? Or is the BC failure not related to this (do you know)?

With permission, I'd also like this PR (or another commit after this PR merges) to bump the version (I'm thinking 2.1, since it shouldn't be breaking). The reason for this request is that I have another package for which I originally implemented this code; with the version bump after the merge, I can have that package depend on "new enough" Qi and remove the code I wrote.

[benknoble]

@countvajhula I am now remembering some things about Racket BC that may make implementing this via qi/macro harder. Any thoughts? Or is the BC failure not related to this (do you know)?

Actually, what I remembered is about 8.3+ vs. 8.2-, so this failure shouldn't be related to BC specifically.

[countvajhula]

@benknoble Thank you for this 💛 . I still need to take a closer look but is there any reason partition couldn't be added as fresh rules in sieve? That would be in line with unifying functionality behind forms that correspond to concepts, e.g. in this case the idea of "dividing values based on properties" seems like a natural generalization of sieve.

Re: the build failure on BC, it sounds a lot like this error, which is a known issue. A workaround is to use racketblock instead of examples. But in the present case, it would be better to sidestep the problem by adding the forms directly into the flow macro, rather than as Qi macros. I've also manually retriggered the CS test just to confirm it isn't anything BC specific.

Re: new version, that sounds like a good reason to cut a new version. As it happens I have been planning a new version for the near future and was envisioning it to include:

• the improvements from the Design Challenge (Fanout form should accept arbitrary Racket expressions #32, Feedback seems to accept _ in the N position #33, Fine-tuning the design of feedback #34). Of these, only Fine-tuning the design of feedback #34 remains to be done.
• a refactor of the flow macro to delegate to component parsers instead of being a single giant parser -- this would make the code easier to work with since each form of the language would have its own parser. That work is coming along nicely, it's in Decompose flow macro into separate parsers for subforms #38 and I'm planning to merge that soon, either right before or after merging in the present PR.
• Baseline performance audit #20 -- Michael Ballantyne has offered to do another pass here once I address his comments from last time - which I plan to do soon.

And this PR would be great to include as well.

I'd say we're pretty close on these. One thing to keep on the radar is that #38 would affect the present PR. It's a straightforward mapping:

1. A single syntax class to handle the matching of all cases for each form, instead of separate clauses in the flow macro
2. The expansion rule simply calls a compile-time function that will expand that particular form
3. This function just gets the input syntax and calls syntax-parse on it, instead of these expansion rules being in the flow macro -- the rules would otherwise be identical

Since the mapping is straightforward, I think we can work on the present PR without worrying about #38. And depending on which one gets merged first, the other can translate this partition code to the new way.

[benknoble]

@benknoble Thank you for this 💛 . I still need to take a closer look but is there any reason partition couldn't be added as fresh rules in sieve? That would be in line with unifying functionality behind forms that correspond to concepts, e.g. in this case the idea of "dividing values based on properties" seems like a natural generalization of sieve.

It is like sieve, but the syntax is more like conds with the brackets. How would you suggest adding rules in sieve that aren't ambiguous? For example: currently sieve accepts 3 clauses (condition, sonex, ronex). Partition accepts arbitrarily many [cond res] clauses. How would we distinguish whether (sieve [a b] [c d] [e f]) is the current sieve or the newer partition-style sieve?

If you want a completely backwards-incompatible break, sieve can become partition, but that seems like a bigger discussion to have.

Until we finalize this, I will keep partition as its own form.

Re: the build failure on BC, it sounds a lot like this error, which is a known issue. A workaround is to use racketblock instead of examples. But in the present case, it would be better to sidestep the problem by adding the forms directly into the flow macro, rather than as Qi macros. I've also manually retriggered the CS test just to confirm it isn't anything BC specific.

Implementing in flow should fix this, as noted.

I'd say we're pretty close on these [PRs for a new version]. One thing to keep on the radar is that #38 would affect the present PR. It's a straightforward mapping:

1. A single syntax class to handle the matching of all cases for each form, instead of separate clauses in the flow macro
2. The expansion rule simply calls a compile-time function that will expand that particular form
3. This function just gets the input syntax and calls syntax-parse on it, instead of these expansion rules being in the flow macro -- the rules would otherwise be identical

Since the mapping is straightforward, I think we can work on the present PR without worrying about #38. And depending on which one gets merged first, the other can translate this partition code to the new way.

Yes, one or the other should be translatable.

[countvajhula]

It is like sieve, but the syntax is more like conds with the brackets. How would you suggest adding rules in sieve that aren't ambiguous? For example: currently sieve accepts 3 clauses (condition, sonex, ronex). Partition accepts arbitrarily many [cond res] clauses. How would we distinguish whether (sieve [a b] [c d] [e f]) is the current sieve or the newer partition-style sieve?

Would putting the (sieve [a b] [c d] [e f]) rule before the existing (sieve cond sonex ronex) rule work? I guess something like (sieve (all positive?) (+ 5) (* 5)) would break this. Yeah, I think you're right that keeping partition separate may be the way to go here. It's similar to if vs cond as you say, or even Qi's if vs switch.

In that case, this needs a fresh form benchmark in profile/forms.rkt -- this comment explains how to add a new benchmark, but basically you can just search that file for some form name (e.g. sieve) and then mirror everything you see there for partition, customizing just the actual functions that do the benchmarking (e.g. run), which mostly involves selecting one of the existing low-level exerciser functions (e.g. check-value or check-values or even both if you want to be fancy and use run-summary-benchmark). You would also need to add the new import in profile/report.rkt, once again by searching for e.g. sieve and mirroring it. Hopefully this'll all make sense when you look at the files!

[benknoble]

Now that tests pass, I'll accept the invite and add the profiling. Then when we have results I'll add the fancier implementation and compare.

RE: naming, sounds good.

[benknoble]

Pushing metrics still isn't working, but the result for the first two commits (naïve impl.) is currently

{"name":"partition","unit":"ms","value":492}

Now we'll see if the partition-values commit works on the first try 😅

[countvajhula]

@benknoble oh man, that's annoying, sorry for the hassle 😖. It would be great to figure out the permissions issue but I think that might take some time since even the official recommendation from github-action-benchmark is that pull requests are not supported. So for now, in order for the charts to show the magic, I'm thinking what we could do is, once this PR is reviewed and ready to go, I can manually cherry-pick the initial version of partition into the main branch prior to merging the PR, and then the rest of the PR can be rebased and then merged into main immediately afterwards. That way, the benchmarks running on main would tell the performance story of partition. That is a bit of a waltz, but if you don't mind, I think it would be good to see that in the charts, and can be an example for anyone else who might consider adding this kind of monitoring to their repo. Wdyt?

[countvajhula]

@benknoble Actually, another, possibly better option: we merge the partition PR with the initial implementation, and then do a second PR for the improved implementation. That way we can use normal workflows instead of cherry-picking, etc.

Thanks for your patience as we iron out the kinks in the performance stuff 😄

[benknoble]

New results:

{"name":"partition","unit":"ms","value":290}

which is definitely an improvement.

---

@countvajhula I'm ok with either approach. Let me know what you want to do—I can close this PR if necessary to open 2 new ones, or re-push it to the original naïve partition + profile and then open a 2nd later for partition-values. You can also capture the profile results by hand from the action :P Like I said, let me know how you want to proceed and I'll be happy to do so, now that I know all the tests are passing and such.

[countvajhula]

@benknoble Awesome! Ok yes, let's do the two-PR approach then. Btw in case you haven't tried already, you can also run tests and profiling locally, via make test, make test-flow, make profile, make profile-forms (running just make shows a full menu). The CI versions are more expansive though (E.g. testing across versions) and reliable (to ensure there aren't any undeclared dependencies).

[benknoble]

Yeah, I know, but the intra- and inter- package requires mean I have to uninstall and reinstall Qi to point to a development copy, and I now have a few local packages that depend on Qi, and it wasn't worth the hassle at the time 😅

[benknoble]

Do you want this to be PR #1 or do you want 2 brand new PRs?

[countvajhula]

I don't have a preference - maybe two new PRs would be easier? Whatever's more convenient for you!

[countvajhula]

nit: Would be good to add some comments to explain how this function works at a high level

[benknoble]

Erm, yes. It was written in a fugue of "flow state" ;) and never got the attention it deserved.