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

Add PositionSortedBy #3543

Merged
merged 1 commit into from Jul 9, 2019

Conversation

fingolfin
Copy link
Member

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.


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 documentation
and tests and all that ;-)

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jul 5, 2019
@coveralls
Copy link

coveralls commented Jul 5, 2019

Coverage Status

Coverage increased (+0.002%) to 84.3% when pulling c372b78 on fingolfin:mh/PositionSortedBy into b5e28d3 on gap-system:master.

Copy link
Member

@wilfwilson wilfwilson left a 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 means i < j => func(list[i]) <= func(list[j])).
  • val should be comparable with the values of func applied to the entries of list
  • "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.
@fingolfin
Copy link
Member Author

@wilfwilson I tried to improve it, please have another look

@codecov
Copy link

codecov bot commented Jul 9, 2019

Codecov Report

Merging #3543 into master will increase coverage by 5.6%.
The diff coverage is 100%.

@@            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
Impacted Files Coverage Δ
lib/list.gd 100% <100%> (ø) ⬆️
src/listfunc.c 97.52% <100%> (+1.8%) ⬆️
lib/list.gi 83.22% <100%> (+2.54%) ⬆️
src/libgap-api.h 50% <0%> (ø)
grp/imf1to9.grp 100% <0%> (ø)
src/julia_gc.c 82.72% <0%> (ø)
lib/teachmod.g 26.73% <0%> (ø)
grp/imf12.grp 100% <0%> (ø)
lib/grpnames.gi 89.42% <0%> (+0.08%) ⬆️
lib/wordrep.gi 81.73% <0%> (+0.09%) ⬆️
... and 203 more

Copy link
Member

@wilfwilson wilfwilson left a 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.

@ChrisJefferson ChrisJefferson merged commit 9499452 into gap-system:master Jul 9, 2019
@fingolfin fingolfin deleted the mh/PositionSortedBy branch July 9, 2019 15:37
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 20, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants