chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.79k stars 420 forks source link

Are we committed to tuple-scalar operator overloads? #23317

Closed bradcray closed 3 months ago

bradcray commented 1 year ago

During @vasslitvinov's demo today, it came up that we support math ops between homogenous tuples and scalars of the same element type. In some ways this seems surprising to me, like that (5, 6, 7) + 1 results in (6, 7, 8) rather than a type mismatch error. In other cases, I think we've found it a nice benefit for conciseness, like if you had a velocity tuple and you multiplied it by a scalar: 5*myVel. Arguably these overloads could be considered a form of promotion, but we don't more generally support promotion for tuples.

This caused both Engin and me to pause a bit and ask "Wait, we support this?" so I thought I'd file this issue to make sure we felt confident it was the right thing.

If I were exploring it, I'd remove the overloads that support this to see where we rely on them today and whether the net use cases made me feel more confident that it's a good thing or more concerned about it.

vasslitvinov commented 1 year ago

Summary: my recommendations are:


Archaelogy: these overloads were most recently added in https://github.com/chapel-lang/chapel/commit/485ee13ceffaddc2d8b34d4e0c7c86efacf64f01, noting that:

these are very natural operators to desire when doing tuple work (like storing velocities and positions as tuples and shifting or scaling them, as in n-body codes), and an annoyance to implement.

The main downside of supporting these operators is that it prevents users from writing their own overloads with the same signatures, if they desire.

Here is my reflection on these comments.

Example:

module Library {
  // an application will typically care about tuples of specific types
  operator +(a: ?k*int, b: real)
    do return "user-defined tuple plus";

  // we can overload the predefined operator with an exact same signature, too
  operator /(x: _tuple, y: x(0).type) where isHomogeneousTuple(x) 
    do return "user-defined tuple divide";
}

module Application {
  proc main {
    test1;
    test2;
  }
  proc test1 {
    writeln((100,200) + 3);    // (103, 203)
    writeln((100,200) / 3);    // (33, 66)
  }
  proc test2 {
    use Library;
    // the same operations as in test1 now use the definitions from Library
    writeln((100,200) + 3);    // user-defined tuple plus
    writeln((100,200) / 3);    // user-defined tuple divide
  }
}

How do we use the tuple-scalar overloads today? As an experiment, I removed them from ChapelTuple.chpl and ran a paratest over test/{release,studies,library}. I got the following "unresolved call" errors:

// const offset = -1 * L * fluff * abstr;
modules/dists/StencilDist.chpl:1366:    '*(1, 3*int(64))' and also 2*int

modules/internal/DefaultSparse.chpl:290:    '-(2*int(64), 1)'

test/release/examples/benchmarks/miniMD/helpers/initMD.chpl:492:    '/(3*real(64), int(64))'
test/release/examples/benchmarks/shootout/mandelbrot-fast.chpl:44:  '*(2.0, 8*real(64))'
test/release/examples/benchmarks/shootout/nbody.chpl:88:            '*(3*real(64), real(64))'
test/release/examples/primers/tuples.chpl:64:                       '+=(3*real(64), real(64))'

test/studies/comd/elegant/arrayOfStructs/util/Util.chpl:56:         '*(3*real(64), real(64))'
test/studies/comd/llnl/CoMD.chpl:18:                                '*(3*real(64), real(64))'

test/studies/shootout/mandelbrot/bradc/mandelbrot-blc.chpl:44:       '*(2.0, 8*real(64))'
test/studies/shootout/mandelbrot/ferguson/mandelbrot-tricks.chpl:50: '*(2.0, 8*real(64))'

test/studies/shootout/nbody/bradc/nbody-blc-ref.chpl:85:    '*(3*real(64), real(64))'
test/studies/shootout/nbody/bradc/nbody-blc-slice.chpl:79:  '*(3*real(64), real(64))'
test/studies/shootout/nbody/bradc/nbody-blc-zip.chpl:81:    '*(3*real(64), real(64))'
test/studies/shootout/nbody/bradc/nbody-blc.chpl:86:        '*(3*real(64), real(64))'
test/studies/shootout/submitted/mandelbrot3.chpl:44:        '*(2.0, 8*real(64))'
test/studies/shootout/submitted/nbody2.chpl:87:             '*(3*real(64), real(64))'
test/studies/shootout/submitted/nbody3.chpl:86:             '*(3*real(64), real(64))'

This is a fairly small number of uses. Also each of these uses looks fairly special to me, so requiring each of these programs to provide the corresponding operator definition makes a good sense to me. OTOH having tuple-scalar operators available "out of the box" also makes sense to me, just like having tuple-tuple operators, so that users do not need to think whether they can do it or not.

vasslitvinov commented 1 year ago

Late-breaking news: in Python, + with tuples means "concatenate" and * with tuples means "replicate." Meaning that Chapel's tuple operations will be non-intuitive for users with Python background. [I do not know how this should affect our decision.]

vasslitvinov commented 1 year ago

No, it is not an annoyance to implement these operators. User implementation is easy.

Oops, I missed the point. The annoyance comes from the lack of a convenient way to promote an operation (an operator or otherwise) across the components of a tuple with an unknown size, such as multiply each tuple component by a given number and return the result as a tuple.

This does not sound like a show-stopper to me. Although this may be because I have written a few of those before so I am used to it by now.

OTOH for the locations produced by my experiment above, I expect that most of them use tuples of a particular size, ex. 3-tuples for 3-d velocity. So user-defined implementations will not hit this issue there.

bradcray commented 1 year ago

Seems like at the very least, we could / arguably should mark them as unstable...

vasslitvinov commented 1 year ago

Yesterday in a poll the team strongly supported marking them unstable. Implementation is forthcoming in #23375.

vasslitvinov commented 1 year ago

We are not committing to providing these overloads for 2.0. I marked them unstable in #23375. Removing the 2.0 tag on this issue.

damianmoz commented 1 year ago

Does Chapel have a broadcast feature, i.e. create an N-tuple from a scalar, e.g.

const x = tuple(1.414. 2);

In #23251, Vass was suggesting that I use a tuple instead of a 2-element 1D array (which does support such operations) for very sound performance reasons. But, I hit a hurdle as soon as I tried to use it.

See my comment there that casting a tuple of real(64) elements to real(32) is an invalid operation. In the light of that instability noted in #23375#, does that pretty much void that approach in the longer term.

I can work around the problem but it makes a tuple of anything other than a real(64) or an int(64) really ugly.

I normally work in real(32) and int(32)/int(16) numbers although I try and make everything generic as much as I can.

vasslitvinov commented 1 year ago

@damianmoz thank you for your input, it is a good argument to stabilize these operators.

Meanwhile it is easy to add the lacking functionality to your code, for example:

// enables (1.0, 1.0): real(32) --> 2*real(32)
inline operator :(x: _tuple, type t:numeric) {
  proc helper(param dim) do
    if dim == 0 then return (x(0):t,);
    else return ((...helper(dim-1)), x(dim):t);
  return helper(x.size-1);
}

// enables tuple(1.414, 2) --> 2*real(64)
proc tuple(arg, param size) {
  var result: size*arg.type;
  for param i in 0..size-1 do result(i) = arg;
  return result;
}
damianmoz commented 1 year ago

Far out. Way beyond my skill set. I will try over the weekend.

Thanks heaps. Paperwork for the rest of the afternoon. So thrilling

damianmoz commented 1 year ago

Still more to do here. I tried to take the absolute value of a tuple and it fails. I then tried a quick fix with

proc abs((x, y) : (real(?w), real(w)))
{
    return (abs(x), abs(y));
}

But that triggers another Chapel bug #23302. In the end, I did the above for real(64) and it runs.

vasslitvinov commented 1 year ago

The abs() example makes me think that we should just allow native promotion over tuple, the same way we do over arrays and other iterables. This way we do not need a ton of overloads of all the standard operators, and in any case the behavior will be easier to explain to users. And we may not need special syntax for "apply this operation to each tuple component and give me back the resulting tuple". @bradcray what do you think?

damianmoz commented 1 year ago

If you are going to make a tuple a viable alternative to using arrays, that all makes a lot of sense. The fact that a lot of operations were not trivially possible with tuples was why I used to avoid them. I also never realised they had so little overhead compared to arrays.

bradcray commented 11 months ago

The abs() example makes me think that we should just allow native promotion over tuple, the same way we do over arrays and other iterables.

I think that's an interesting proposal. I have trouble knowing whether it would feel like a good thing or like a potential large source of confusion. Supporting in-place / fixed size arrays and using those when promotion is desired would be another obvious approach.

ShreyasKhandekar commented 7 months ago

Stabilizing Tuple Scalar Operations

Preparing for the discussion about these operations and making them stable for 2.0

What is the API being presented?

Tuple scalar operators are ones which take a tuple and a scalar as arguments to produce a result. For example: (1, 2, 3) + 1

The operators are:

+  -  *  /  %  **  &  |  ^  <<  >>

How is it intended to be used?

The intended use was ambiguous, which is why these operators were marked as unstable. Adapted from Brad's example above:

It seems surprising, that (5, 6, 7) + 1 results in (6, 7, 8) rather than a type mismatch error; however, in other cases, I think we've found it a nice benefit for conciseness, like if you had a velocity tuple and you multiplied it by a scalar: 5 * myVel. Arguably these overloads could be considered a form of promotion, but we don't more generally support promotion for tuples.

How is it being used in Arkouda and CHAMPS?

In Arkouda and CHAMPS:

In release/examples:

These are also used in our Tutorial slides.

What's the history of the feature?

These operators were first added in 2007 by https://github.com/chapel-lang/chapel/commit/1db924e95122bf1915e8e5830b3a466ba78592d6 then backed out due to the implementation approach rather than the utility/design concerns. They were added back in 2013 by https://github.com/chapel-lang/chapel/commit/485ee13ceffaddc2d8b34d4e0c7c86efacf64f01 in ChapelTuple.chpl. Details about the reasons for backing out of the change originally can be found in the commit message for https://github.com/chapel-lang/chapel/commit/485ee13ceffaddc2d8b34d4e0c7c86efacf64f01.

The same commit message also has some motivation about why these were added:

these are very natural operators to desire when doing tuple work (like storing velocities and positions as tuples and shifting or scaling them, as in n-body codes), and an annoyance to implement.

The functionality was expanded to support Coercible types with https://github.com/chapel-lang/chapel/commit/77510730df4dae08e4615993d7140a34d663dc3c

We didn't add support for equality related operators due to concerns that it such cases may represent programmer error rather than an intention promote an equality test across the tuple components. This support is up for reconsideration but can be added post 2.0.

And of course these were marked unstable by https://github.com/chapel-lang/chapel/pull/23375

What's the precedent in other languages, if they support it?

a. Python

Python tuples support + as concatenation and * as repetition.

b. C/C++

C++ has std::tuple which does not support any of the binary operators that we have right now. But it supports comparison operators which lexicographically compares the values in the tuple. These include ==, !=, <= , >=, <, >, <=> where only == and <=> are still supported in C++20 and the rest were removed.

C++ has tuple_cat to concatenate tuples instead of an operator.

c. Rust

Rust has no binary operators on tuples.

d. Swift

Swift only has comparison operators for tuple-tuple comparisons.

e. Julia

Same as Swift

f. Go

Doesn't have tuples (Wow!) and plans to not introduce them either.

Are there known Github issues with the feature?

None except this one

Are there features it is related to? What impact would changing or adding this feature have on those other features?

These are not tied into other features that I can see

How do you propose to solve the problem?

Here are a few things we can do:

Option 1

We provide the existing operations for 2.0 but off by default. a. We remove the operations that were marked unstable from ChapelTuple.chpl b. They can be enabled by importing or useing a new standard/package module made for this purpose. TupleScalarOps.chpl maybe?

Option 2

Allow promotion for tuple operations in General. a. This way they match more closely with arrays and we don't have to worry about individually adding support for each operator. b. This might cast a wider net than we intent do, and our current intention to support in-place arrays makes this less appealing in terms of tuple-functionality.

Option 3

Stabilize as is a. Do nothing special, just make them stable as is and make them available by default

Option 4

Get rid of them altogether. Users must provide their own.

I want to go with Option 1 here.

bradcray commented 7 months ago

My feeling on this today is that I think that we should move toward a world in which all types with a these() iterator are candidates for promotion (https://github.com/chapel-lang/chapel/issues/5114) and that since homogeneous tuples support a these() iterator, they should support promotion generally. However, I think option 2 is a bit of a heavy lift for us to chase after at present, so my thought is to do option 3 for now and pursue option 2 in the future, anticipating it as being a mostly non-breaking change.

ShreyasKhandekar commented 7 months ago

From today's meeting we reached the following conclusion:

Concern for providing these operations stemmed from the surprise of finding out about their existence. Therefore, with an abundance of caution, these were marked unstable. Now, the surprise and concerns have both diminished. Everyone on this sub team seems to be okay with a combination of Option 2 and 3 above (4-nil vote) which is : Option 3: Stabilize as is now, Option 2 (future): We intend to support promotions at a later time frame. See https://github.com/chapel-lang/chapel/issues/24682

ShreyasKhandekar commented 3 months ago

With these overloads being stabilized and question of promotion already captured in a spinoff issue. I think we can close this.