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

feat: add Sine + Cosine + Signbit #805

Merged
merged 5 commits into from
Jun 6, 2023
Merged

Conversation

schollz
Copy link
Contributor

@schollz schollz commented May 7, 2023

Description

This PR adds in the math.Sin and math.Cos methods. This file is from the Pre-Go 1 history: https://github.com/golang/go/blob/e4de2e7fd04c92d4035cd268d5043f2380aef437/src/pkg/math/sin.go The future versions had trouble with the gno language due to unsupported features.

How has this been tested?

Yes, I am using it in a realm that I am working on (https://github.com/schollz/gno/tree/bytebeat). There are no tests for the math packages, but I can add tests if needed.

This version of sin.go comes from pre Go1. Futures versions rely on unsafe which is not yet implemented in gnovm
@schollz schollz requested a review from a team as a code owner May 7, 2023 17:03
@schollz schollz mentioned this pull request May 7, 2023
@moul moul requested a review from jaekwon May 7, 2023 22:32
Copy link
Member

@albttx albttx left a comment

Choose a reason for hiding this comment

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

Even if the code is from Go, Could you add some tests to ensure future stability ? :)

@schollz
Copy link
Contributor Author

schollz commented May 10, 2023

@albttx Yes, it never hurts! I can add some tests.

@schollz schollz requested a review from moul as a code owner May 14, 2023 21:54
@schollz schollz changed the title feat: add Sine / Cosine feat: add Sine + Cosine + Signbit May 14, 2023
@schollz
Copy link
Contributor Author

schollz commented May 14, 2023

Tests added for Signbit, Sin, Cos. @moul - shall I continue to add all the tests for the current math Gno packages?

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good 💯

Thank you for the addition 🙏

@moul
Copy link
Member

moul commented May 17, 2023

I suggest adding a new compatibility documentation file and using this PR as an example for updating helpers while maintaining the compatibility matrix. Please wait a bit longer before merging.

@ajnavarro ajnavarro added 🌱 feature New update to Gno 🧾 package/realm Tag used for new Realms or Packages. labels May 18, 2023
@moul
Copy link
Member

moul commented May 19, 2023

schollz added a commit to schollz/gno that referenced this pull request May 22, 2023
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related and removed 🧾 package/realm Tag used for new Realms or Packages. labels May 22, 2023
@schollz
Copy link
Contributor Author

schollz commented May 22, 2023

@jaekwon
Copy link
Contributor

jaekwon commented Jun 2, 2023

reading now

Copy link
Contributor

@jaekwon jaekwon left a comment

Choose a reason for hiding this comment

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

nice. feels good.

@ajnavarro
Copy link
Contributor

pinging @moul with the final vote to merge this.

@moul moul merged commit 128a5d8 into gnolang:master Jun 6, 2023
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related 🌱 feature New update to Gno
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants