Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#236 - Generalize continuous-subseqs #294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeff303
Copy link
Contributor

@jeff303 jeff303 commented Aug 14, 2020

Adding new subseq-pred-fn macro to create the new form of predicate function (taking the previous and current item), to preserve backward compatibility (still allowing predicate functions that only take the current item)

Adding SubseqsDynamicPredFn, which works the same as SrangeEndFn, to support backward compatibility

Adding wrapper to take a predicate on [prev current] and turn it into a predicate also taking the current index as the first param

Creating transducer to combine this with the user-supplied predicate function

Adding tests for select and transform

WORK IN PROGRESS

TODO: figure out how to make predicate function handle an open-ended subsequence (ex: end marker not yet seen)

@jeff303
Copy link
Contributor Author

jeff303 commented Aug 14, 2020

Adding a currently failing test to spark a discussion on how best to handle this scenario (i.e. creating a predicate function that correctly handles seeing only the beginning, but not ending, of a subsequence).

@@ -960,7 +960,26 @@
(is (= [[] [2] [4 6]]
(select
[(s/continuous-subseqs number?) (s/filterer even?)]
[1 "a" "b" 2 3 "c" 4 5 6 "d" "e" "f"]))))
[1 "a" "b" 2 3 "c" 4 5 6 "d" "e" "f"])))
(defn- make-bounds-pred-fn [start end]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments here:

  • The defn should be a top-level form
  • I'm struggling to understand this function. Shouldn't it be returning true or false? Why does it return start and end in some cases. Is prev the previous element or true/false for the run of this function on the previous element?

Copy link
Contributor Author

@jeff303 jeff303 Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a little dicey. Basically, it's trying to have the "previous" value do double duty: both signaling that the item should be included in a selected subsequence (i.e. true), but also carrying the previous boundary keyword (if applicable), without indicating that keyword should be included in a subsequence. So, more or less the same concept as outlined in the issue #236 description.

I tried changing the predicate around so that it returns a vector of both of these separately, but it seems to get a bit gnarly trying to handle that in the transducer:

(let [index-results (into [] (subseq-pred-fn-transducer p) aseq)]
  (map last (filter (comp true? first) index-results)))

I suppose the caller could be asked to supply an additional function to subseq-pred-fn that controls how the truthy piece is pulled out of the results for the purpose of building the actual subsequence indeces?

function (taking the previous and current item), to preserve backward
compatibility (still allowing predicate functions that only take the
current item).  This macro also takes in a get-truthy-fn as its first
argument, which is a function that marks whether that item in the
sequence should be included in a subsequence.  This is necessary
because the predicate function can now be of any arbitrary form, so
we cannot make any assumption about how the user intends for that
result to be interpreted as a "filter".

Adding SubseqsDynamicPredFn, which works the same as SrangeEndFn, to support backward compatibility

Adding wrapper to take a predicate on [prev current] and turn it into a predicate also taking the current index as the first param

Creating transducer to combine this with the user-supplied predicate function

Adding tests for select and transform

TODO: figure out how to make predicate function handle an open-ended subsequence (ex: end marker not yet seen)
@jeff303
Copy link
Contributor Author

jeff303 commented Sep 25, 2020

I took another crack at this, this time adding an additional parameter to the macro so the user can specify their "truthy function".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants