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(stdlib): Add asin, acos, atan, isClose to Number module #1699

Merged
merged 14 commits into from
Feb 1, 2024

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Feb 23, 2023

This pr adds Number.asin, Number.acos and Number.isClose to the Number stdlib. I was just going to add asin and acos but given that the methods being used to calculate asin and acos are approximations I decided to also add isClose to make the testing more concise.

isClose is taken from https://peps.python.org/pep-0485/#proposed-implementation,
The license on it just says its in the public domain I think that means we can use it but I honestly dont know happy to find an alternative approach though if that isnt allowed.

The asin and acos functions are from:
https://git.musl-libc.org/cgit/musl/tree/src/math/asin.c
Just like pow.

This is a draft because isClose is blocked on optional Arguments for a nice api, once that gets merged I will go back and refactor the function to not use Options.

Blocked: #388

Work for: #1017

Note: if you guys are fine with merging in with a worse api based on options I am happy todo that too.

stdlib/number.gr Outdated Show resolved Hide resolved
@spotandjake
Copy link
Member Author

Note: refactor any tests that use <> to arround floating points to use isClose after we are unblocked.

@spotandjake spotandjake force-pushed the spotandjake-trig-funcs-isClose branch 2 times, most recently from afa10b4 to 251e4ed Compare February 28, 2023 23:50
@spotandjake spotandjake changed the title feat: Add asin, acos, isClose to Number stdlib feat: Add asin, acos, atan, isClose to Number stdlib Mar 1, 2023
@spotandjake spotandjake self-assigned this Mar 1, 2023
@spotandjake spotandjake force-pushed the spotandjake-trig-funcs-isClose branch from 251e4ed to ea44db1 Compare March 15, 2023 19:51
@spotandjake spotandjake marked this pull request as ready for review March 15, 2023 20:02
@spotandjake spotandjake changed the title feat: Add asin, acos, atan, isClose to Number stdlib feat(stdlib): Add asin, acos, atan, isClose to Number stdlib Mar 18, 2023
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/number.gr Outdated
Comment on lines 425 to 426
* @param relativeTolerance: The relative tolerance to use
* @param absoluteTolerance: The absolute tolerance to use
Copy link
Member

Choose a reason for hiding this comment

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

These should probably be more descriptive because it's not clear what they do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to improve these and got them a bit better but i dont know if they are good enough yet,

https://docs.python.org/3/library/math.html I was looking at python's docs for the description.

@spotandjake
Copy link
Member Author

I think this might also close: #303 if we are deciding to with oscar's approach anyway.

@spotandjake
Copy link
Member Author

This is blocked by #1776

@spotandjake spotandjake force-pushed the spotandjake-trig-funcs-isClose branch 2 times, most recently from 12872af to 323e276 Compare December 29, 2023 20:36
@spotandjake
Copy link
Member Author

This has been unblocked and the docs have been regenerated

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

I love the new descriptions! I think they make a lot of sense now. Just one tiny change and this looks good from me.

stdlib/number.gr Outdated Show resolved Hide resolved
Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

Stamp of approval from me! I'll let the stdlib expert @phated take a look and hit the merge button if he's happy with the docs.

@phated
Copy link
Member

phated commented Jan 27, 2024

@spotandjake I only see a copyright notice, but not a license. What's the license on the code you referred to?

@spotandjake
Copy link
Member Author

@spotandjake I only see a copyright notice, but not a license. What's the license on the code you referred to?

From my understanding as it's in the Public domain it doesn't need a license to be used. It seems to be unlicensed though.

I'm happy to use a different implementation if we need to though.

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Comment about docs. I still need to double check the musl libc license

stdlib/number.gr Outdated
* Computes the inverse tangent of the given angle
*
* @param angle: A number between -1 and 1, representing the angle's tangent value.
* @returns The inverse tangent (angle in radians between `-pi/2` and `pi/2`) of x. If x is less than `-1` or greater than `1`, returns NaN
Copy link
Member

Choose a reason for hiding this comment

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

This is for all the returns in the file.

What is "x"? We usually say something like "of the given angle."

Also, the "if x is less" sentence needs to be something like "The result will be NaN if the given angle is not between -1 and 1." Or something similar

Copy link
Member Author

Choose a reason for hiding this comment

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

I made that change

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

More docs changes

stdlib/number.gr Outdated
* @param b: The second value
* @param relativeTolerance: The maximum tolerance to use relative to the larger absolute value `a` or `b`
* @param absoluteTolerance: The absolute tolerance to use, regardless of the values of `a` or `b`
*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*

stdlib/number.gr Outdated
p / q
}
/**
* Computes the inverse sine of the given angle
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Computes the inverse sine of the given angle
* Computes the inverse sine of the given angle.

Period on top level descriptions, no period on arg descriptions

stdlib/number.gr Outdated
/**
* Computes the inverse sine of the given angle
*
* @param angle: A number between -1 and 1, representing the angle's sine value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param angle: A number between -1 and 1, representing the angle's sine value.
* @param angle: A number between -1 and 1, representing the angle's sine value

stdlib/number.gr Outdated
* Computes the inverse sine of the given angle
*
* @param angle: A number between -1 and 1, representing the angle's sine value.
* @returns The inverse sine (angle in radians between `-pi/2` and `pi/2`) of the given `angle`. The result will be `NaN` if the given `angle` is less than `-1` or greater than `1`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @returns The inverse sine (angle in radians between `-pi/2` and `pi/2`) of the given `angle`. The result will be `NaN` if the given `angle` is less than `-1` or greater than `1`
* @returns The inverse sine (angle in radians between `-pi/2` and `pi/2`) of the given `angle` or `NaN` if the given `angle` is not between `-1` and `1`

How about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Love It

stdlib/number.gr Outdated
}

/**
* Computes the inverse cosine of the given angle
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Computes the inverse cosine of the given angle
* Computes the inverse cosine of the given angle.

stdlib/number.gr Outdated
/**
* Computes the inverse cosine of the given angle
*
* @param angle: A number between -1 and 1, representing the angle's cosine value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param angle: A number between -1 and 1, representing the angle's cosine value.
* @param angle: A number between -1 and 1, representing the angle's cosine value

stdlib/number.gr Outdated
* Computes the inverse cosine of the given angle
*
* @param angle: A number between -1 and 1, representing the angle's cosine value.
* @returnsThe inverse cosine (angle in radians between `-pi/2` and `pi/2`) of the given `angle`. The result will be `NaN` if the given `angle` is less than `-1` or greater than `1`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @returnsThe inverse cosine (angle in radians between `-pi/2` and `pi/2`) of the given `angle`. The result will be `NaN` if the given `angle` is less than `-1` or greater than `1`
* @returns The inverse cosine (angle in radians between `-pi/2` and `pi/2`) of the given `angle` or `NaN` if the given `angle` is not between `-1` and `1`

stdlib/number.gr Outdated
}

/**
* Computes the inverse tangent of the given angle
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Computes the inverse tangent of the given angle
* Computes the inverse tangent of the given angle.

stdlib/number.gr Outdated
/**
* Computes the inverse tangent of the given angle
*
* @param angle: A number between -1 and 1, representing the angle's tangent value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param angle: A number between -1 and 1, representing the angle's tangent value.
* @param angle: A number between -1 and 1, representing the angle's tangent value

stdlib/number.gr Outdated
* Computes the inverse tangent of the given angle
*
* @param angle: A number between -1 and 1, representing the angle's tangent value.
* @returns The inverse tangent (angle in radians between `-pi/2` and `pi/2`) of the given `angle`. The result will be `NaN` if the given `angle` is less than `-1` or greater than `1`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @returns The inverse tangent (angle in radians between `-pi/2` and `pi/2`) of the given `angle`. The result will be `NaN` if the given `angle` is less than `-1` or greater than `1`
* @returns The inverse tangent (angle in radians between `-pi/2` and `pi/2`) of the given `angle` or `NaN` if the given `angle` is not between `-1` and `1`

@spotandjake
Copy link
Member Author

Made those changes

@phated phated changed the title feat(stdlib): Add asin, acos, atan, isClose to Number stdlib feat(stdlib): Add asin, acos, atan, isClose to Number module Feb 1, 2024
@phated phated added this pull request to the merge queue Feb 1, 2024
Merged via the queue into grain-lang:main with commit 353b544 Feb 1, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants