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 Number.tan, Number.factorial, Number.clamp, Number.lerp #1373

Closed
wants to merge 7 commits into from

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Jul 26, 2022

This pr adds Number.tan, Number.factorial, Number.clamp, Number.lerp to the Number libary for #1017

@spotandjake spotandjake changed the title feat(stdlib): Add Number.tan and Number.factorial feat(stdlib): Add Number.tan, Number.factorial, Number.clamp, Number.lerp, Number.map Jul 26, 2022
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/number.gr Show resolved Hide resolved
stdlib/number.gr Outdated Show resolved Hide resolved
stdlib/number.gr Outdated Show resolved Hide resolved
spotandjake and others added 4 commits July 31, 2022 15:56
- Undo Changes To GitIgnore
- Update Doc Versions
- Update Docs On Lerp
Rewrite Number.Factorial To Use Recurrsion
@spotandjake
Copy link
Member Author

I made the changes we talked about in discord i will move Number.map to its own pr i wasnt able to run the tests because downloading the modules on my internet is taking forever.

@spotandjake spotandjake changed the title feat(stdlib): Add Number.tan, Number.factorial, Number.clamp, Number.lerp, Number.map feat(stdlib): Add Number.tan, Number.factorial, Number.clamp, Number.lerp Aug 4, 2022
Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
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.

I'm having some issues with this stuff.

/**
* Computes the factorial of a number.
*
* @param input: The number to compute the factorial of, Must be a Non negative integer
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 input: The number to compute the factorial of, Must be a Non negative integer
* @param input: The non-negative integer used to compute the factorial

*/
export let factorial = input => {
// Error If It is Negative, or Not an Integer
if (sign(input) is -1 || isInteger(input)) {
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
if (sign(input) is -1 || isInteger(input)) {
if (input < 0 || isInteger(input)) {

this is better, right?

export let factorial = input => {
// Error If It is Negative, or Not an Integer
if (sign(input) is -1 || isInteger(input)) {
fail "Factorial Expects A Non Negative Integer Input"
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually want to fail or return an option? @ospencer

Copy link
Member

Choose a reason for hiding this comment

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

It should fail. If we really don't want it to fail then we can take Jake's suggestion to define this as the gamma function for non-integers.

fail "Factorial Expects A Non Negative Integer Input"
} else {
// Factorial Helper
let rec fact = n => if (n is 0) 1 else n * fact(n - 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
let rec fact = n => if (n is 0) 1 else n * fact(n - 1)
let rec fact = n => if (n == 0) 1 else n * fact(n - 1)

why use is?

Comment on lines +478 to +479
* @param min: The minimum value in the range
* @param max: The maximum value in the range
Copy link
Member

Choose a reason for hiding this comment

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

Did someone say Range?

*
* @since v0.5.3
*/
export let lerp = (start: Number, end: Number, amount: Number) => {
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
export let lerp = (start: Number, end: Number, amount: Number) => {
export let linearInterpret = (start: Number, end: Number, amount: Number) => {

Hear me out. lerp sounds like I'm 🤮

*
* @param start: The starting value
* @param end: The ending value
* @param amount: an amount between 0 and 1, anything outside the range will produce an extrapolation i.e `lerp(0, 100, 2)` will produce 200
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 a "interpolation" but you are saying that it also "exptrapolates". I don't like that.

@spotandjake
Copy link
Member Author

spotandjake commented Aug 5, 2022

can someone convert this to a draft pr until #1404 is completed

@spotandjake
Copy link
Member Author

spotandjake commented Dec 8, 2022

I think i am going to close this as it will require an entire rewrite to use range and the scope of this pr has changed to just be map, lerp and clamp. Going to add a comment to the issue though linking to this as this has some stuff related to documenting that is still useful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants