-
Notifications
You must be signed in to change notification settings - Fork 258
Improvements to parallel assignment #3748
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
base: development
Are you sure you want to change the base?
Conversation
I don't love |
Yeah I don't expect anyone would use parallel assignment much for method installation. It just seems like something that ought to work if we want = and := to work in general. Also, it lets us put parentheses around a single expression for grouping, solving the |
fae8011
to
b06c309
Compare
e104064
to
2ae336d
Compare
I'm gonna unsubscribe because there are a lot of force pushes but the notifications don't show what has actually changed. @ me when it is ready. |
This will allow us to figure out the symbol later for augmented/parallel assignment. Also merge "unary" and "postfix" members of Symbol struct. A symbol can't have both anyway, and since we're storing the symbol and not the specific function, we wouldn't be sure which we want.
Then we can use this info for augmented/parallel assignment.
This info is now saved in the lhs Code object
Now contains a CodeSequence of the elements on the left hand side instead of symbol information. This will enable us to support other types of code in the left hand side.
The left hand side is now a sequence of Code objects rather than a list of symbols. Also add support for (x) = y (behaves just like x = y).
e.g., assignment to mutable hash tables and lists and installing methods
There was likely a typo in the file, and the last input likely was supposed to be b and not a. This apparently was "fixed" at some point, as this is the behavior in Macaulay2 1.24.11: i1 : (b) = 1:x o1 = 1 : (x) o1 : Sequence i2 : b o2 = x o2 : Symbol However, this seems incorrect, as (b) is not a sequence in Macaulay2, but just b itself surrounded by parentheses for grouping.
2ae336d
to
fffd641
Compare
if nlhs == 1 then value = seq(value); | ||
if nlhs == 1 then ( | ||
when value is a:Sequence do ( | ||
-- unless y is a sequence of length 1, then it behaves like x = y#0 |
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.
Just to make sure, (x) = (1,2,3)
should still be an error, correct?
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.
In the current draft, it's equivalent to x = (1,2,3)
. But for consistency, you're right -- it probably should be an error. I'll push a new version here in a moment.
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.
@d-torrance @mahrud Is this agreed on, should I take a separate look?
bb0cc07
to
89b8108
Compare
One more thing, either for now or a future PR: would be great if this kind of parallel assignment also worked: i1 : for (a,b) in {(1,2), (3,4)} list a
stdio:1:4:(3): error: expected symbol |
Ooh yeah that would be nice! I played around with it for a bit and it would require a lot of pretty low-level changes to binding and whatnot. Let's hold off for a future PR. |
export setuppostfix(e:SymbolClosure,fn:unop):void := ( | ||
unopNameList = unopNameListCell(fn,e.symbol,unopNameList); | ||
e.symbol.postfix = fn; | ||
); |
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 understand what this commit does, but was it necessary? I see the simplification, but also seems like not a bad idea to be able to differentiate postfix vs unary operators in the interpreter.
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.
Once we're done parsing, there's really no difference between prefix and postfix unary operators. We just stuff the corresponding function in the unaryCode
object and call it a day. And a unary operator can't be both prefix and postfix anyway -- it has to be one or the other. So it didn't seem necessary to have two separate fields in the Symbol
struct.
Also, with this PR I'm proposing the unaryCode
objects store the operator symbol itself (for potential use by parallel/augmented assignment) instead of the function that gets called. And if the symbol has both a prefix and postfix function that might get called, we'd have to do some extra stuff to figure out which one to use.
It should still be possible to differentiate between prefix and postfix by looking at the parseinfo
member of the corresponding Word
object.
@@ -210,7 +210,6 @@ export augmentedAssignmentCode := {+ | |||
oper:Symbol, | |||
lhs:Code, | |||
rhs:Code, | |||
info:Symbol, -- variable name or operator |
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.
Is there any reason to keep this for debugging purposes?
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.
This was a hack that I came up when adding augmented assignment. There were two unrelated cases where we needed some information about a symbol that wasn't being stored in the Code object:
- The variable name for
globalMemoryReferenceCode
andthreadMemoryReferenceCode
, which currently just store the frame index of the variable. - The binary/unary operator for
binaryCode
andunaryCode
, which currently just store the function associated with the operator.
With this PR, the proposal is that these various Code objects now store the symbols themselves. So the symbol that was in info
is still there, but now inside lhs
.
M2/Macaulay2/m2/integers.m2
Outdated
@@ -116,6 +116,7 @@ scan({symbol +=, symbol -=, symbol &=, symbol |=, symbol ^^=}, | |||
|
|||
store = method() | |||
store(AtomicInt, ZZ) := atomicStore | |||
(<- AtomicInt) := store |
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.
Why is this method defined this way?
{ "If the left hand side has more elements than the right hand side, then the extra symbols on the left side are given the value ", TO "null", "." }, | ||
{ "If the left hand side has fewer elements than the right hand side, then the last symbol on the left hand side is given | ||
as value a sequence containing the trailing elements of the right hand side." | ||
}, | ||
{ "If the right hand side is not a sequence, then it is assigned to the first symbol on the left, and the remaining symbols are assigned the | ||
value ", TO "null", "." | ||
} | ||
{ "the number of expressions must match the number of variables." } |
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 was this documented behavior removed? It actually seems preferable to me.
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.
No idea. I was surprised to learn about it!
It probably wouldn't be too much trouble to restore it.
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.
The amount of changes is not overwhelming, but I don't feel confident in thoroughly reviewing this before the next release. I think we should merge this after the release, so any potential bugs have some time to reveal themselves.
Also, several aspects of this touch on things that @pzinn has more experience with, so his review may also be valuable.
I'm totally fine with waiting for the next release. |
How much harder is it to also allow |
Adding support for the various types of visible list on the right hand side would be very simple, I think. Ensuring that the types match would be a bit trickier since we currently don't keep track of which type of parentheses we're using on the left hand side. |
Okay, I'm totally fine with |
I just pushed a proposal to allow other types of list on the right hand side. Currently, it always returns a sequence, since the easy way to implement this is just to extract the sequence from the list and use it. i1 : {x, y, z} = {1, 2, 3}
o1 = (1, 2, 3)
o1 : Sequence |
We don't actually give any new examples for = and := since "multiple" assignment is documented before the other uses of these operators, so it would seem out of order. But we do mention that it's possible to use it.
f2ea270
to
a5a2c3b
Compare
Thinking about this more, I think we want |
a5a2c3b
to
8fc0a87
Compare
Done! i1 : {x, y, z} = {1, 2, 3}
o1 = {1, 2, 3}
o1 : List |
If we've reached the assignment stage during x ??= y, we know that x is nullish (otherwise we would have returned it earlier). But we've been running "x = (x ?? y)", even though we know that the right hand side will be y in this case.
@d-torrance once Paul's PR is merged, shall we merge this as well? |
Sounds good to me! |
We add various parallel assignment features, e.g:
Parallel assignment for mutable hash tables and lists
Partial parallel assignment
Parallel method installation
Parallel augmented assignment
Edit: I decided against
(x,y,z) += 1
being equivalent to(x,y,z) += (1,1, 1)
.Fixes precedence issues with assignment
See #3733
TODO:
>>
docs to mention that we need parentheses for installing methodsCloses: #1774