add math module #357

b5 · 2021-03-05T19:02:37Z

closes #78.

cc @dustmop, @adonovan, & @essobedo

lib/math/math.go

lib/math/trig.go

starlark/testdata/math.star

srebhan

Hooray another one! :-) Thanks @b5 for pushing this!
Looks almost perfect. Maybe just rename floatFunc to floatFunc1 to match the other name and add the in radians comment to the documentation of all trigonometric functions. It's missing for some functions at the end of the file. But it's only my two cent...

essobedo · 2021-03-06T06:30:29Z

@b5 I take it over

google-cla · 2021-03-06T07:52:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

essobedo · 2021-03-06T07:53:02Z

@googlebot I consent.

essobedo · 2021-03-06T07:58:31Z

@adonovan I believe that I have made all the requested changes except Let's call this abs, and overload it for int and float.. I tried something regarding this remark, but I don't like it, could you please tell me what you had in mind exactly?

lib/math/math.go

essobedo · 2021-03-08T09:44:22Z

@adonovan Fixed, please check again

lib/math/math.go

starlark/testdata/math.star

essobedo · 2021-03-08T14:29:54Z

@adonovan all fixed, please check again

essobedo · 2021-03-08T14:35:11Z

It sounds that there is an issue with the build, the status is never reported

lib/math/math.go

essobedo · 2021-03-08T17:41:43Z

@adonovan done, please check it again

lib/math/math.go

essobedo · 2021-03-08T21:09:49Z

@adonovan Done, I also added some new functions

essobedo · 2021-03-09T08:11:59Z

@adonovan done, please check again

adonovan

Nearly there!

lib/math/math.go

essobedo · 2021-03-09T14:27:48Z

@adonovan done

adonovan

Looks good. Many thanks.

adonovan · 2021-03-09T14:29:27Z

Jon, can you merge this PR? It LGTM.

b5 · 2021-03-09T20:04:08Z

Thanks again @adonovan & @essobedo! Excited to keep these moving

brandjon · 2021-03-10T18:10:04Z

The status check for CI never returned. Can you try force-pushing a new commit (e.g. with git commit --amend --no-edit), as suggested here?

brandjon · 2021-03-10T18:12:42Z

Actually looks like our travis CI integration method is deprecated. Will reconfigure on my side...

brandjon · 2021-03-10T18:17:04Z

Actually it looks like we've received status updates recently from this hook and the deprecation that article's talking about happened a while ago, so let's just force push and see if that fixes it.

b5 · 2021-03-10T18:20:53Z

ok, force pushed, doesn't look like we're having any luck ☹️

fenollp · 2021-03-11T17:41:40Z

Hi! Sorry to barge in a little late.
I see the type floatOrInt float64 was introduced to Unpack arguments of all builtins.
Are we not loosing precision here? We seem to be discarding anything that doesn't fit in float64 (so big.Ints).
But I don't see this mentioned. Did I miss it?

Am I correct in assuming that Unpacking a litteral bigint into float64 returns an error?

abs(9007199254740992+1) # = abs(1+2^53) does this fail?

adonovan · 2021-03-11T18:21:28Z

I see the type floatOrInt float64 was introduced to Unpack arguments of all builtins.
Are we not loosing precision here? We seem to be discarding anything that doesn't fit in float64 (so big.Ints).

An int of any magnitude is converted to a float. This will fail if the magnitude is bigger than about 1e308. However, the result of math.abs is always a floating-point number, even if the argument was an integer.

This is consistent with Python3, but it does argue in favor of renaming math.abs to math.fabs, both for consistency with Python, and to indicate that the result is a float. I'm not aware of how to compute abs of a big int in Python. We should also have a test for this.

UPDATE: Ah, Python's abs function (not in math package) returns the abs value without changing the type. Perhaps this should be added to the spec as a standard "universe" function.

tetromino · 2021-03-12T23:14:51Z

I manually kicked off the travis build, and it finally worked.

But now it seems we are missing the google cla check from one of the authors.

@essobedo and @b5 - please see googlebot's comment above-791891376 - you would need to make a comment with the literal words @googlebot I consent. to agree to the cla.

tetromino · 2021-03-12T23:15:48Z

Never mind, googlebot is now happy :)

brandjon · 2021-03-13T01:01:45Z

Thanks for taking care of the merge @tetromino.

…am versions After a *large* community effort, we've successfully moved two packages from starlib into the "real" standard library in go-starlark: * math: google/starlark-go#357 * time: google/starlark-go#327 Both of these packages have had a great deal of vetting & improvement in the process of migrating. While this does introduce many breaking changes for both packages, both packages have better stability & performance characteristics. Moving forward, we'll try to follow the pattern of developing & testing packages here, and for those deemed worthy, move them up into go-starlark. Once a package lands there, we'll switch starlib to being a strict import-and-document-only, which effectively locks their API. Keeping package APIs locked will cut down on drift, benefitting the broader starlark ecosystem. BREAKING CHANGE: math & time modules have been overhauled. Refer to package documentation for details

…am versions After a *large* community effort, we've successfully moved two packages from starlib into the "real" standard library in go-starlark: * math: google/starlark-go#357 * time: google/starlark-go#327 In addition, both time & math imports have lost thier ".star" suffixes: load("time.star", "time") -> load("time", "time") load("math.star, "math") -> load("math", "math") Both of these packages have had a great deal of vetting & improvement in the process of migrating. While this does introduce many breaking changes for both packages, both packages have better stability & performance characteristics. Moving forward, we'll try to follow the pattern of developing & testing packages here, and for those deemed worthy, move them up into go-starlark. Once a package lands there, we'll switch starlib to being a strict import-and-document-only, which effectively locks their API. Keeping package APIs locked will cut down on drift, benefitting the broader starlark ecosystem. BREAKING CHANGE: math & time modules have been overhauled. Refer to package documentation for details

add math module

be118f1

adonovan reviewed Mar 5, 2021

View reviewed changes

srebhan reviewed Mar 5, 2021

View reviewed changes

math: Applied remarks

f8a3101

essobedo force-pushed the feat_math_module branch from 43666d8 to f8a3101 Compare March 6, 2021 08:14

adonovan reviewed Mar 7, 2021

View reviewed changes

math: Applied remarks part 2

00e681c

essobedo force-pushed the feat_math_module branch from a0ebf52 to 00e681c Compare March 8, 2021 09:43

adonovan reviewed Mar 8, 2021

View reviewed changes

math: Applied remarks part 3

b19ffec

adonovan reviewed Mar 8, 2021

View reviewed changes

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

adonovan reviewed Mar 8, 2021

View reviewed changes

lib/math/math.go Outdated Show resolved Hide resolved

math: Applied remarks part 4

c1b7e05

essobedo force-pushed the feat_math_module branch from 364dfe1 to c1b7e05 Compare March 8, 2021 17:51

adonovan reviewed Mar 8, 2021

View reviewed changes

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

math: Add missing functions

a33965d

math: Use a dedicated function for log

899dc83

adonovan reviewed Mar 9, 2021

View reviewed changes

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

lib/math/math.go Outdated Show resolved Hide resolved

adonovan approved these changes Mar 9, 2021

View reviewed changes

adonovan assigned brandjon Mar 9, 2021

time: Inline the log function

3327e5d

b5 force-pushed the feat_math_module branch from 2a9903d to 3327e5d Compare March 10, 2021 18:19

adonovan mentioned this pull request Mar 11, 2021

spec: add abs(int or float) function to compute absolute value bazelbuild/starlark#183

Closed

tetromino merged commit 74c10e2 into google:master Mar 12, 2021

essobedo deleted the feat_math_module branch March 15, 2021 08:26

adonovan mentioned this pull request Mar 15, 2021

lib/math: rename abs to fabs #362

Closed

b5 mentioned this pull request Apr 8, 2021

refactor(math,time): replace math & time modules with Starlark upstream versions qri-io/starlib#68

Merged

b5 mentioned this pull request Apr 29, 2021

chore(release): release v0.5.0 qri-io/starlib#72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add math module #357

add math module #357

b5 commented Mar 5, 2021 •

edited

Loading

srebhan left a comment

essobedo commented Mar 6, 2021

google-cla bot commented Mar 6, 2021

essobedo commented Mar 6, 2021

essobedo commented Mar 6, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 9, 2021

adonovan left a comment

essobedo commented Mar 9, 2021

adonovan left a comment

adonovan commented Mar 9, 2021

b5 commented Mar 9, 2021

brandjon commented Mar 10, 2021

brandjon commented Mar 10, 2021

brandjon commented Mar 10, 2021

b5 commented Mar 10, 2021

fenollp commented Mar 11, 2021

adonovan commented Mar 11, 2021 •

edited

Loading

tetromino commented Mar 12, 2021

tetromino commented Mar 12, 2021

brandjon commented Mar 13, 2021 •

edited

Loading

add math module #357

add math module #357

Conversation

b5 commented Mar 5, 2021 • edited Loading

srebhan left a comment

Choose a reason for hiding this comment

essobedo commented Mar 6, 2021

google-cla bot commented Mar 6, 2021

essobedo commented Mar 6, 2021

essobedo commented Mar 6, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 8, 2021

essobedo commented Mar 9, 2021

adonovan left a comment

Choose a reason for hiding this comment

essobedo commented Mar 9, 2021

adonovan left a comment

Choose a reason for hiding this comment

adonovan commented Mar 9, 2021

b5 commented Mar 9, 2021

brandjon commented Mar 10, 2021

brandjon commented Mar 10, 2021

brandjon commented Mar 10, 2021

b5 commented Mar 10, 2021

fenollp commented Mar 11, 2021

adonovan commented Mar 11, 2021 • edited Loading

tetromino commented Mar 12, 2021

tetromino commented Mar 12, 2021

brandjon commented Mar 13, 2021 • edited Loading

b5 commented Mar 5, 2021 •

edited

Loading

adonovan commented Mar 11, 2021 •

edited

Loading

brandjon commented Mar 13, 2021 •

edited

Loading