-
Notifications
You must be signed in to change notification settings - Fork 238
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 several pairs-related methods #3617
Conversation
718cf4a
to
709e641
Compare
M2/Macaulay2/d/actors3.d
Outdated
-- # typical value: applyPairs, BasicList, Function, List | ||
-- # typical value: applyPairs, Dictionary, Function, List | ||
-- # typical value: applyPairs, Thing, Function, Iterator |
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 it would be better to list these closer to where they are handled (e.g. near mappairs
or map
. We don't currently do this, but I'd like to have locate
also show the location of compiled functions in the interpreter (the location is stored in tvalues.m2
as a comment right now).
Outputs | ||
L:List | ||
L:{List, Iterator} |
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 it might be better to separate the documentation of the iterator form, ideally using Synopsis
.
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.
Cool -- I didn't know about Synopsis
! Definitely useful for stuff like this.
It's a pretty common error message
applyPairs(x, f) is the same as apply(pairs x, f)
scanPairs(x, f) is the same as scan(pairs x, f)
selectPairs(x, f) is the same as select(pairs x, f) Doesn't support iterators (yet) since select is defined for them at top-level.
Now specify that they take BasicList arguments and move from typicalvalues.m2. Also document missing function variant.
709e641
to
7f4070b
Compare
I added a few more commits updating |
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.
Looks good to me. I left some comments, but most of them are rather subjective and don't need to block merging.
when r | ||
is s:Sequence do list(o.Class, s, o.Mutable) | ||
else buildErrorPacket("internal error; expected a sequence")) | ||
else applyEE(getGlobalVariable(pairsIteratorS), e)); -- # typical value: pairs, Thing, Iterator |
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.
fwiw I think putting the typical value
tag on its own line would be better.
r := pairs(Expr(o.v)); | ||
when r | ||
is s:Sequence do list(o.Class, s, o.Mutable) | ||
else buildErrorPacket("internal error; expected a sequence")) |
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.
when would pairs(Expr(o.v))
with o:List
fail?
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.
It shouldn't -- we should always get a sequence here. I don't expect we'll ever raise this error.
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.
Hmm, unless pairs
raises an error... I'll look into this more.
@@ -2346,6 +2363,20 @@ scan(e:Expr):Expr := ( | |||
else WrongNumArgs(2)); | |||
setupfun("scan",scan); | |||
|
|||
-- # typical value: scanPairs, Thing, Function, Nothing |
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.
Since this line is for readability of documentation and doesn't have to be literally true, I wonder if it would be useful to use a dummy Iterable
class instead of Thing
for things like this. I know that practically anything with an iterator
method defined on it can be iterable, so it may cause some confusion.
-- # typical value: selectPairs, HashTable, Function, HashTable | ||
-- # typical value: selectPairs, ZZ, HashTable, Function, HashTable | ||
selectPairs(nval:int, obj:HashTable, f:Expr):Expr := ( |
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 at some point we need to decide on a convention about where to place these lines. I don't find placing both of these here particularly useful. To me it either means one of them should be defined on the top level, or one of them is in the wrong place.
export WrongArgHashTable():Expr := WrongArg("a hash table"); | ||
export WrongArgHashTable(n:int):Expr := WrongArg(n, "a hash table"); | ||
export WrongArgImmutableHashTable():Expr := WrongArg("an immutable hash table"); | ||
export WrongArgImmutableHashTable(n:int):Expr := WrongArg(n, "an immutable hash table"); |
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.
Seems to me like WrongArgHashTable
should say immutable hash table
and we should have a WrongArgMutableHashTable
which says mutable hash table
.
We add
pairs
for any class with aniterator
method as well asapplyPairs
,scanPairs
, andselectPairs
for non-hash tables. These are just shortcuts forapply(pairs ...)
,scan(pairs ...)
, andselect(pairs ...)
.selectPairs
doesn't currently work for iterators sinceselect
is defined for them at top-level, and this is all done in the interpreter.Closes: #3603