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 sum() to List and Array #36

Closed
wants to merge 1 commit into from
Closed

Add sum() to List and Array #36

wants to merge 1 commit into from

Conversation

fantix
Copy link
Contributor

@fantix fantix commented Mar 9, 2024

I'm not sure if we even want the sum() functions directly on data structures, it seems like a better abstraction if we instead put it on an iterator (like Rust), only that we don't have iterators yet. So for testing/early-adopting purposes, I think it's fine to have them as demonstrated in this PR, even if it means we need to deprecate and remove them later.

Ported from muts.

Comment on lines 18 to 14
/// Trait for types that override the `+` add operator.
pub trait Add {
op_add(Self, Self) -> Self
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda feel this doesn't belong here, but I couldn't find an existing Add trait or a better place to put it.

@bobzhang
Copy link
Contributor

is providing fold in #25 not enough?

@Yoorkin
Copy link
Collaborator

Yoorkin commented Mar 11, 2024

IMO we should provide such utility based on some general function and traits.
Here is a design from Haskell:

trait Num {
  op_add(Self,Self) -> Self
  op_sub(Self,Self) -> Self
  op_div(Self,Self) -> Self
  op_mul(Self,Self) -> Self
  op_neg(Self) -> Self
  abs(Self) -> Self
  sign_num(Self) -> Int
}

fn op_add(self : Int, that : Int) -> Int { self + that }
fn op_sub(self : Int, that : Int) -> Int { self - that }
fn op_div(self : Int, that : Int) -> Int { self / that }
fn op_mul(self : Int, that : Int) -> Int { self * that }
fn op_neg(self : Int) -> Int { - self }
fn abs(self : Int) -> Int { if self < 0 { - self } else { self } }
fn sign_num(self : Int) -> Int { 
  if self > 0 { 1 }
  else if self < 0 { -1 }
  else { 0 }  
}

fn sum[T : Num + Default](ls : List[T]) -> T {
  ls.fold(T::default(), T::op_add)
}

test {
  let a = List::[1,2,3,4]
  @assertion.assert_eq(sum(a),10)?
}

The implementation of sum is based on the Num trait and @list.fold (we could also provide product in this way). Should we also add the Semigroup and Monoid traits? It require more discussion and design.

@bobzhang
Copy link
Contributor

what's the status of this PR?

@fantix
Copy link
Contributor Author

fantix commented Mar 29, 2024

what's the status of this PR?

I just rebased this PR, but I don't have enough knowledge to mark it ready, please see my comments below.

is providing fold in #25 not enough?

It's mostly a question for the owners - how "battery-included" do we want to make the stdlib?

I believe sum() and product() are handy reductions on iterable sequences for stdlibs, even though they are just deadly simple wrappers around fold() implementations. As a comparison, Python has built-in sum() and math.prod(), while Rust has Sum and Product under std::iter. It's debatable that both sample languages are not functional enough (like hiding reduce() under functools), but I believe the engineering goal is similar.

IMO we should provide such utility based on some general function and traits.

I agree, quoting myself: "it seems like a better abstraction if we instead put it on an iterator (like Rust), only that we don't have iterators yet".

However, I have some different opinions towards adding the Num trait (thanks for adding it anyways!). It's a bit too strict for users who only want to customize a subset of the trait. For instance, an add-only struct can't benefit from the sum() shortcut in ths PR:

struct MyNum {
  v: Int
} derive(Debug, Default)

fn MyNum::op_add(x: MyNum, y: MyNum) -> MyNum {
  MyNum::{ v: x.v + y.v }
}

fn main {
  let x = MyNum::{ v: 3 }
  let y = MyNum::{ v: 5 }
  debug(x + y)  // this is fine

  let l = [x, y]
  debug(l.sum())  // this is not
}

Compilation fails:

main/main.mbt:15:11-15:14 Type MyNum does not implement trait @moonbitlang/core/num.Num:
  - method from_int is missing
  - method op_sub is missing
  - method op_mul is missing
  - method op_div is missing
  - method op_neg is missing
  - method abs is missing
  - method signum is missing

So basically, I'm proposing that we tear down the Num trait into finer-grained traits like Add and Multiply, and probably migrate them into the math package (added after the num package). The Num trait may stay as an aggregation of such base traits. (Proposed in #165)

Should we also add the Semigroup and Monoid traits? It require more discussion and design.

This doesn't seem like a question for me, but my two cents for this is a mild "no" based on my limited FP knowledge and heavy Python background.

I prefer the Rust-style combinators in a general iterator interface available over most sequential data structures. Examples would be the iter mooncake from @tonyfettes, or the iter package from myself.

Actually, this is part of a bigger design question about how we want to organize the "duplicate" code where we have a separate sum() for each of List and Array, and also possibly Vec and Deque and so on. It seems like it's not considered a code smell because such duplicates are very short and do not tend to change over time. I kinda agree with @bobzhang saying "其实一辈子只写一次的 我们都鼓励写重复性代码" (sorry for quoting without permission), it's only that such similar but different APIs are not friendly enough for library authors who want to interface against generic sequences.

Perhaps the long-awaited generic trait would solve this issue without introducing heavy logistics to get it going. Presumably, a general sum() implementation could be as simple as:

trait Foldable[T] {
  fold[U](Self[T], initial: U, f: (U, T) -> U) -> U
}

impl Foldable[T: Default + @math.Add]::sum(self: Self) -> T {
  self.fold(T::default(), fn { acc, x => acc + x })
}

To summarize, I'm not sure about the next steps. We have several options, and I'll be happy to create PRs once we decide on the direction(s):

  • Drop sum() from stdlib and let mooncakes do it
  • Add FP-style traits like Semigroup and Monoid
  • Wait for generic trait feature
  • Land this PR as it is now for testing/early-adopting purposes
    • ... and add sum()/product() to vec and other friends too
  • Add an @iter package and support general sequences ([PoC] Dynamic iterator demo #167)
  • Break down @num.Num into @math.Add + @math.Multiply + ... = @math.Num (Split and move @num.Num into @math.Num parts #165)

@@ -2,6 +2,8 @@
"import": [
"moonbitlang/core/assertion",
"moonbitlang/core/math",
"moonbitlang/core/num",
"moonbitlang/core/int",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

int should be a test_import only.

@Lampese
Copy link
Collaborator

Lampese commented Mar 31, 2024

what's the status of this PR?

I just rebased this PR, but I don't have enough knowledge to mark it ready, please see my comments below.

is providing fold in #25 not enough?

It's mostly a question for the owners - how "battery-included" do we want to make the stdlib?

I believe sum() and product() are handy reductions on iterable sequences for stdlibs, even though they are just deadly simple wrappers around fold() implementations. As a comparison, Python has built-in sum() and math.prod(), while Rust has Sum and Product under std::iter. It's debatable that both sample languages are not functional enough (like hiding reduce() under functools), but I believe the engineering goal is similar.

IMO we should provide such utility based on some general function and traits.

I agree, quoting myself: "it seems like a better abstraction if we instead put it on an iterator (like Rust), only that we don't have iterators yet".

However, I have some different opinions towards adding the Num trait (thanks for adding it anyways!). It's a bit too strict for users who only want to customize a subset of the trait. For instance, an add-only struct can't benefit from the sum() shortcut in ths PR:

struct MyNum {
  v: Int
} derive(Debug, Default)

fn MyNum::op_add(x: MyNum, y: MyNum) -> MyNum {
  MyNum::{ v: x.v + y.v }
}

fn main {
  let x = MyNum::{ v: 3 }
  let y = MyNum::{ v: 5 }
  debug(x + y)  // this is fine

  let l = [x, y]
  debug(l.sum())  // this is not
}

Compilation fails:

main/main.mbt:15:11-15:14 Type MyNum does not implement trait @moonbitlang/core/num.Num:
  - method from_int is missing
  - method op_sub is missing
  - method op_mul is missing
  - method op_div is missing
  - method op_neg is missing
  - method abs is missing
  - method signum is missing

So basically, I'm proposing that we tear down the Num trait into finer-grained traits like Add and Multiply, and probably migrate them into the math package (added after the num package). The Num trait may stay as an aggregation of such base traits. (Proposed in #165)

Should we also add the Semigroup and Monoid traits? It require more discussion and design.

This doesn't seem like a question for me, but my two cents for this is a mild "no" based on my limited FP knowledge and heavy Python background.

I prefer the Rust-style combinators in a general iterator interface available over most sequential data structures. Examples would be the iter mooncake from @tonyfettes, or the iter package from myself.

Actually, this is part of a bigger design question about how we want to organize the "duplicate" code where we have a separate sum() for each of List and Array, and also possibly Vec and Deque and so on. It seems like it's not considered a code smell because such duplicates are very short and do not tend to change over time. I kinda agree with @bobzhang saying "其实一辈子只写一次的 我们都鼓励写重复性代码" (sorry for quoting without permission), it's only that such similar but different APIs are not friendly enough for library authors who want to interface against generic sequences.

Perhaps the long-awaited generic trait would solve this issue without introducing heavy logistics to get it going. Presumably, a general sum() implementation could be as simple as:

trait Foldable[T] {
  fold[U](Self[T], initial: U, f: (U, T) -> U) -> U
}

impl Foldable[T: Default + @math.Add]::sum(self: Self) -> T {
  self.fold(T::default(), fn { acc, x => acc + x })
}

To summarize, I'm not sure about the next steps. We have several options, and I'll be happy to create PRs once we decide on the direction(s):

  • Drop sum() from stdlib and let mooncakes do it
  • Add FP-style traits like Semigroup and Monoid
  • Wait for generic trait feature
  • Land this PR as it is now for testing/early-adopting purposes
    • ... and add sum()/product() to vec and other friends too
  • Add an @iter package and support general sequences ([PoC] Dynamic iterator demo #167)
  • Break down @num.Num into @math.Add + @math.Multiply + ... = @math.Num (Split and move @num.Num into @math.Num parts #165)

The generic trait seems like a great solution. Now a lot of places can't go without it.

@fantix
Copy link
Contributor Author

fantix commented Apr 27, 2024

  • Add an @iter package and support general sequences

Feels like this is the path we're going down now, so I'll close this PR in favor of adding @iter.Iter::sum() on top of @iter.Iter::fold().

The generic trait seems like a great solution. Now a lot of places can't go without it.

And to add the generic trait is still appreciated ✌️

@fantix fantix closed this Apr 27, 2024
@fantix fantix mentioned this pull request Apr 27, 2024
1 task
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.

4 participants