zip core form
#190
zip core form
#190
Conversation
countvajhula
had a problem deploying
to
test-env
GitHub Actions
Failure
qi-test/tests/flow.rkt
Outdated
| (test-equal? "zip with primitive operation" | ||
| ((☯ (~> (zip +) ▽)) | ||
| '(1 2) '(3 4)) | ||
| '(4 6)) | ||
| ;; (test-equal? "zip with flow operation" | ||
| ;; ((☯ (~> (zip (~> string->number +)) ▽)) | ||
| ;; '("1" "2") '("3" "4")) | ||
| ;; '(4 6)) |
There was a problem hiding this comment.
These both look like (map proc multiple inputs), although the second is slightly more interesting (which you discovered when fixing it in the next commit):
> ,r qi
> (~>> ('(1 2) '(3 4)) (map +) sep)
4
6
> (~>> ('("1" "2") '("3" "4")) (map (flow (~> (>< string->number) +))) sep)
4
6There was a problem hiding this comment.
I coincidentally just edited the PR description to add this:
"One connection to the current release is that qi/list map does not accept multiple arguments and does not have the "zip" like behavior of Racket's map. Even in the future when we perhaps support multiple values in map, the result of applying map to multiple list inputs might not be zip-like, and instead, could produce multiple outputs. This zip form could serve the need in such cases."
So we're effectively removing the zip behavior from (or more precisely, not providing such behavior in our version of) map and restoring it via a distinct form.
I suppose if we end up deciding that qi/list map should provide zip-like behavior on multiple values like Racket's map, then at that point that could make zip redundant.
There was a problem hiding this comment.
FWIW, it seems likely we will deviate from Racket map in this respect since we independently arrived at this amp-like behavior via both "flowy logic" and in our recent discussions about "rank polymorphic Qi". So if this line of thinking pans out, this zip being introduced is unlikely to become redundant.
There was a problem hiding this comment.
It looks like in the original problem described #183 (modified from @soegaard's posed problem) , modeling the proposed zip behavior using Racket map would look like this:
(define (dup-sqr v)
(values (sqr v) (sqr v)))
(~> (1 2 3) (>< (~> dup-sqr ▽)) zip)
(~> (1 2 3) (>< (~> dup-sqr ▽)) (map list __) △)
It seems desirable to be able to do this within Qi though (and looks cleaner here).
This supports zipping with any operation, defaulting to list if none is indicated. Towards drym-org#183.
I'm not sure if it can be expressed using the a priori Qi core language, and in that case, it needs to be added as a new core language form and not as a macro.
It appears that △ and ▽ are closely related to the usual `zip` and `unzip` operations in functional languages and represent a boundary condition in their semantics. Therefore, it's better to remove the tentative new `zip` core form in favor of generalizing the existing △ form (and in the future, likewise, also generalizing ▽). This does entail a small backwards incompatibility, as `(△ flo)` formerly accepted a single list and any number of additional values that would be passed to each invocation of flo along with each element of the list. That behavior is intuitively very similar to the "zip" behavior, except that we now (more properly, it would seem) simply expect these additional values to be present in separate input lists, and these values may _happen_ to all be the same at each index.
countvajhula
had a problem deploying
to
test-env
GitHub Actions
Failure
We don't currently have a benchmark for this "zip" behavior of △, but modifying the existing local benchmark would invalidate the trends for that benchmark.
countvajhula
had a problem deploying
to
test-env
GitHub Actions
Failure
|
Here's what changed between range-diff: |
| [_:id #'(λ args | ||
| (if (singleton? args) | ||
| (let ([v (first args)]) | ||
| (if (list? v) | ||
| (apply values v) ; fast path to the basic △ behavior | ||
| (raise-argument-error '△ | ||
| "list?" | ||
| v))) | ||
| (apply (qi0->racket (△ _)) args)))])) |
There was a problem hiding this comment.
Maybe I'm missing it, but this seems like a change from the original :id clause that isn't mentioned by the commit or docs; perhaps it intended to be a separate kind of change "optimize common (sep)"?
There was a problem hiding this comment.
The original code expected a single list input, so I think this change was just to accommodate potentially more than one input. But I may have ended up optimizing it a bit by writing it fully in Racket? Not sure. The benchmark for sep did show a slight improvement I think.
There was a problem hiding this comment.
Oh, right. I guess I was looking at the "fast path" bit more than the overall computation.
| (let ([vs (map first seqs)]) | ||
| (append (values->list (apply op vs)) | ||
| (~zip-with op (map rest seqs) truncate)))))) | ||
| (if (exists empty? seqs) |
There was a problem hiding this comment.
Probably a slight optimization over doing the null check ourselves, seems reasonable. Notably the empty check also appears in the caller zip-with.
There was a problem hiding this comment.
The caller null check checks whether seqs is empty, whereas this one checks if any of the members of seqs is empty. We also have the caller of the caller do what feels like a redundant singleton? check before re-packing the arguments into a list (in the id case of sep). I seemed to conclude that was necessary because if we do it within zip-with, we already have lost the "fast path" option for the common case. I jumbled things around for a while before settling on this implementation somewhat empirically, but I wouldn't be surprised if it could be improved.
|
Sounds good, we'll leave time to verify any code breakage before merging. Let's also notify others, @NoahStoryM @chansey97 -- this PR contains a small backwards incompatibility in use of the |
|
Btw, I'd love to merge this before next week's meeting (Jan 24) if possible (that is, assuming there are no objections), so that we can add the new release tag and announce the release before then 😄 . I'm especially curious to learn your perspective on this change @NoahStoryM |
|
Reminder: please review this PR by tomorrow if possible 🙏 |
|
From my initial tests, neither benknoble/advent2021 nor the Frosthaven Manager project is affected by this change (I got lucky: I think I'm only ever passing a single list to |
|
@benknoble Thank you for checking! Btw, if you're only using a single list with |
Yes, that's precisely why I'd been using it, for the |
Summary of Changes
This was originally to add a
zipcore form. But upon further reflection it emerged thatzipis actually a generalization ofsep, and likewise,unzipis a generalization ofcollect!This PR therefore generalizes those existing forms (just
sepfor now) rather than introduce new ones.The solution to the original problem in #183 is now:
We could potentially provide a macro called
zipthat expands to(△ ▽), if we feel that would be useful, or we could just learn to recognize this as a phrase (mentioned in the documentation in this PR).Original PR description follows, for historical context:
I started this a little while back to address #183 . It isn't related to most of the changes in the Qi 5 release branch, but if we feel it's OK to include, then we might as well.
One connection to the current release is that
qi/listmapdoes not accept multiple arguments and does not have the "zip" like behavior of Racket'smap. Even in the future when we perhaps support multiple values inmap, the result of applying map to multiple list inputs might not be zip-like, and instead, could produce multiple outputs. Thiszipform could serve the need in such cases.We'd initially discussed that
zipwould be part ofqi/list, but upon further reflection, as it does not appear to reduce to any other core form of the language, it seems to make more sense for it to be a core form. I did consider whetherrelaycould be expressed aszipor vice versa, but seemed to conclude that they cannot be mutually expressed.Public Domain Dedication
(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.)