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 PositionMaximum and PositionMinimum #956

Merged

Conversation

ChrisJefferson
Copy link
Contributor

  • [ X ] Adds new features

Write below the description of changes (for the release notes)

This adds the new methods PositionMaximum and PositionMinimum, which returns the index of the largest element of a list. It also can be used to find the largest element under some function.

I have had this function privately for some time, and often use it myself. For example, finding the largest element of a list l of groups can be done by l[PositionMaximum(l, Size)], whereas I don't know of any other short way of doing this in GAP (if I've missed a method somewhere, please tell me).

@codecov-io
Copy link

codecov-io commented Nov 21, 2016

Current coverage is 48.75% (diff: 100%)

Merging #956 into master will decrease coverage by 0.02%

@@             master       #956   diff @@
==========================================
  Files           424        424          
  Lines        222125     222167    +42   
  Methods        3430       3430          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         108344     108315    -29   
- Misses       113781     113852    +71   
  Partials          0          0          

Powered by Codecov. Last update 2908745...7e57162

@frankluebeck
Copy link
Member

Looks sensible and fine to me.
I've certainly written this code within a bigger function more than once.

## <#GAPDoc Label="PositionMaximum">
## <ManSection>
## <Oper Name="PositionMaximum" Arg='list [, func]'/>
## <Oper Name="PositionMinimum" Arg='list [, func]'/>
Copy link
Member

Choose a reason for hiding this comment

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

It does not invalidate GAPDoc, but since both PositionMaximum and PositionMinimum are functions, it would be more consistent to use Func instead of Oper in <Oper Name="PositionMaximum" Arg='list [, func]'/> (there is even an automated check for such things added in #538).

if Length(args) = 2 then
func := args[2];
else
func := x -> x;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use IdFunc here as in the previous function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly should use IdFunc here

bestindex := fail;
for i in [ 1 .. Length( list ) ] do
if IsBound( list[i] ) then
ival := func ( list[ i ] );
Copy link
Member

Choose a reason for hiding this comment

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

Should we be concerned about performance and treat the 1-argument case differently to avoid calling IdFunc on each element of the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IdFunc is very fast. It might speed it up a little, but at the cost of twice as much, almost identical, code.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then let's fix GAPDoc elements and use IdFunc in both functions, and merge this...

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

4 participants