Skip to content

PR: Add reciprocal cube root#502

Merged
kgryte merged 13 commits into
developfrom
rcbrt
Jul 21, 2022
Merged

PR: Add reciprocal cube root#502
kgryte merged 13 commits into
developfrom
rcbrt

Conversation

@steff456

Copy link
Copy Markdown
Contributor

Resolves #222

Description

This PR adds a package for reciprocal cube root

What is the purpose of this pull request?

This pull request:

  • Creates the functionality for calling rcbrt

Related Issues

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@steff456
steff456 requested review from Planeshifter and kgryte July 20, 2022 21:19
@steff456 steff456 self-assigned this Jul 20, 2022

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉 Welcome! 🎉

And thank you for opening your first pull request! We will review it shortly. 🏃 💨

Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/README.md Outdated

@kgryte kgryte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, @steff456!

Remaining items:

  • updating copyright
  • address comments

I'll commit the description changes (i.e., s/cubic/cube/).

Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/README.md Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/README.md Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/README.md Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/benchmark/benchmark.js Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/test/test.native.js Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/test/test.native.js Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/test/test.native.js Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/test/test.native.js Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/test/test.native.js Outdated

@kgryte kgryte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@steff456 If you could copy over the tests for negative values, that would be great!

Note, however, that you'll need to add the opts argument to each test, as the native tests can only be run in Node.js environments, not browser environments.

@kgryte kgryte added the Math Issue or pull request specific to math functionality. label Jul 21, 2022
@kgryte

kgryte commented Jul 21, 2022

Copy link
Copy Markdown
Member

May also do a once over to ensure that I was able to scrub all cubic usage and changed to cube.

Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/docs/types/index.d.ts Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/README.md Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/README.md Outdated
Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/README.md Outdated
@kgryte

kgryte commented Jul 21, 2022

Copy link
Copy Markdown
Member

One last thing, would you mind renaming the SVG file such that s/cubic/cube/?

Comment thread lib/node_modules/@stdlib/math/base/special/rcbrt/test/test.native.js Outdated
@kgryte

kgryte commented Jul 21, 2022

Copy link
Copy Markdown
Member

Awesome! Only renaming the SVG file and adding the missing opts argument remain. Otherwise, this PR is good for merge.

@kgryte kgryte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks, @steff456!

@kgryte
kgryte merged commit 1e7990e into develop Jul 21, 2022
@kgryte
kgryte deleted the rcbrt branch July 21, 2022 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Math Issue or pull request specific to math functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RFC: Add support for computing the reciprocal cube root (rcbrt)

2 participants