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

Math migration #18

Merged
merged 23 commits into from
Mar 25, 2022
Merged

Conversation

JamieBallingall
Copy link
Contributor

@JamieBallingall JamieBallingall commented Aug 23, 2021

Introduces many of the functions currently living in purescript-math. Closes #17. See also purescript-deprecated/purescript-math#31.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

src/Data/Number.purs Outdated Show resolved Hide resolved
README.md Outdated
@@ -59,6 +59,122 @@ true
false
```

Remainder:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realise that the readme already had examples in, but I think examples should go into the doc-comments instead, so that they can be seen on Pursuit.

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 agree about examples in the doc-comments. I'd be happy to completely rework the README to give a shorter and more general overview of the functions Data.Number supports but that will mean changing what was already there before this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds good to me, thanks!

Copy link
Contributor Author

@JamieBallingall JamieBallingall Jan 15, 2022

Choose a reason for hiding this comment

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

After some delay (my apologies), this should now be resolved in 626e299

README.md Outdated

Trignometric functions:
```purs
> sin 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have an opportunity to clarify that sin takes radians here; how about using pi/2 as the value?

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 agree but I missed this in the last commit. I'll fix shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 73c1e25

src/Data/Number.purs Outdated Show resolved Hide resolved
src/Data/Number.purs Outdated Show resolved Hide resolved
test/Test/Main.purs Outdated Show resolved Hide resolved
@JamieBallingall JamieBallingall mentioned this pull request Nov 9, 2021
4 tasks
@JamieBallingall
Copy link
Contributor Author

@hdgarrood Over on purescript/purescript#2980 you express hesitancy on polyfills that mutate the Math object. This repo contains a polyfill for sign that reads:

exports.sign = Math.sign ? Math.sign : function(x) {
  return x === 0 || x !== x ? x : (x < 0 ? -1 : 1);
};

That doesn't look to me like it mutates the Math object but I'm certainly no expert on Javascript. If you have concerns over sign we can remove the Javascript implementation and implement sign directly in Purescript, at some (theoretical) performance cost?

@hdgarrood
Copy link
Collaborator

No, that looks fine! That won’t mutate Math, it’ll just use Math.sign if it exists and otherwise it’ll use the polyfill.

@JamieBallingall
Copy link
Contributor Author

One final tweak. I've switched trunc to work that same way as sign -- using the Math object if available with a polyfill backup.

@JordanMartinez JordanMartinez added purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump. labels Mar 2, 2022
JordanMartinez
JordanMartinez previously approved these changes Mar 17, 2022
Copy link
Contributor

@JordanMartinez JordanMartinez left a comment

Choose a reason for hiding this comment

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

LGTM!

@JordanMartinez JordanMartinez dismissed their stale review March 25, 2022 15:34

As noted in the Math issue (my comment), we need to decide whether to replicate more functions added to the Math object in ES6.

@JordanMartinez
Copy link
Contributor

🏓 @thomashoneyman This should be ready to go now.

@thomashoneyman
Copy link
Member

I haven't been involved in these discussions, so I'd appreciate if perhaps @hdgarrood could take another look. If not, I can review, but I don't necessarily have the correct background to make these decisions.

Copy link
Collaborator

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

I have a couple of minor docs suggestions but this look great 👍


-- | Returns `e` exponentiated to the power of the argument.
-- | ```purs
-- | > exp 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like exp 1.0 might be a better example here; any function of the form \x -> pow a x for some a is going to take 0.0 to 1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in latest commit.


-- | Returns the natural logarithm of a number.
-- | ```purs
-- | > log 1.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here - maybe log e would be more illustrative here? I realise you have that same example on the documentation for e but I think repetition here is fine, especially as people might be linked directly to the documentation for exp and log and not see e.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in latest commit.

@JordanMartinez JordanMartinez merged commit 2d1a223 into purescript:master Mar 25, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
purs-0.15 A reminder to address this issue or merge this PR before we release PureScript v0.15.0 type: breaking change A change that requires a major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement functions currently in purescript-math
4 participants