-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add PositionSortedBy #3543
Add PositionSortedBy #3543
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the functionality.
In my opinion, several things should be made more explicit in the documentation (as they are in the documentation of Sort
/SortBy
/PositionSorted
, as appropriate). Feel free to take on board whichever you like:
func
has to be a one-argument function (sorted meansi < j => func(list[i]) <= func(list[j])
).val
should be comparable with the values offunc
applied to the entries oflist
- "the position at which an object should be inserted"... such that the list is still sorted (or the first position where such an object already exists, etc etc).
You could also consider doing something like describing it this way:
PositionSortedBy([x1, x2, ..., xn], val, func) = PositionSorted([func(x1), func(x2), ..., func(xn)], val)
(under the assumptions that [func(x1), func(x2), ..., func(xn)]
is already sorted w.r.t. <=
and val
is comparable with its entries).
One thing to point out is that the interface is a bit different from PositionSorted, which might be confusing: the object you pass in is *not* a (potential) element of the list, but rather the result of applying the "by func" to an object (or at least something that looks like such a result). The reason: first off, replacing the element by its func value would be the very first thing done by any implementation anyway. And a typical application of this function is to search for an object matching a a certain criterion -- say, the first item which is a list whose first entry is 42. With the current model, one can write PositionSortedBy([ "a", "ab" ], 4, Length); and is not forced to first construct an actual object, like here: PositionSortedBy([ "a", "ab" ], "abcd", Length); In this example, that's a pretty small saving, but in bigger examples, it can add up. In particular, this is motivated by real world usage of this function.
14857c8
to
c372b78
Compare
@wilfwilson I tried to improve it, please have another look |
Codecov Report
@@ Coverage Diff @@
## master #3543 +/- ##
==========================================
+ Coverage 78.91% 84.52% +5.6%
==========================================
Files 691 696 +5
Lines 341505 345061 +3556
==========================================
+ Hits 269494 291656 +22162
+ Misses 72011 53405 -18606
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's really nice! Thanks for the changes @fingolfin.
One thing to point out is that the interface is a bit different from
PositionSorted, which might be confusing: the object you pass in is
not a (potential) element of the list, but rather the result of
applying the "by func" to an object (or at least something that looks
like such a result). The reason: first off, replacing the element by its
func value would be the very first thing done by any implementation
anyway. And a typical application of this function is to search for an
object matching a a certain criterion -- say, the first item which is a
list whose first entry is 42. With the current model, one can write
and is not forced to first construct an actual object, like here:
In this example, that's a pretty small saving, but in bigger examples,
it can add up. In particular, this is motivated by real world usage
of this function.
By the way, this is perhaps my oldest still active branch for GAP: I wrote
it in early 2014, around the time I discovered
PositionFirstComponent
and tried to get rid of that (which finally happened this year). I then never
could get myself to clean this up, but today I wanted to have such a function
in
recog
, so I finally had a good enough reason to dig in and write documentationand tests and all that ;-)