Skip to content

Conversation

@JithinKS97
Copy link
Contributor

@JithinKS97 JithinKS97 commented Jun 24, 2020

Resolves #233 .

Checklist

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

  • Read, understood, and followed the contributing guidelines, including the relevant style guides.
  • Read and understand the Code of Conduct.
  • Read and understood the licensing terms.
  • Searched for existing issues and pull requests before submitting this pull request.
  • Filed an issue (or an issue already existed) prior to submitting this pull request.
  • Rebased onto latest develop.
  • Submitted against develop branch.

Description

What is the purpose of this pull request?

This pull request:

  • Add support for computing the inverse/arc cotangent (acot)

Related Issues

Does this pull request have any related issues?

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.

The test fixtures for the arccot function were generated using Julia 1.4.2 where as in the other parts of the code base, an older version of 0.5 is used and the function range is used instead of linspace as linspace is deprecated in the newer version of julia.


@stdlib-js/reviewers

@JithinKS97 JithinKS97 changed the title Added Inverse cotangent to math functions. Added Inverse cotangent to math/base/special Jun 24, 2020
@JithinKS97 JithinKS97 changed the title Added Inverse cotangent to math/base/special Added Inverse cotangent to @stdlib/math/base/special Jun 24, 2020
@JithinKS97 JithinKS97 changed the title Added Inverse cotangent to @stdlib/math/base/special Added acot to @stdlib/math/base/special Jun 24, 2020
@JithinKS97 JithinKS97 changed the title Added acot to @stdlib/math/base/special Add support for computing the inverse/arc cotangent (acot) Jun 24, 2020
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality. Needs Review A pull request which needs code review. labels Jun 24, 2020
@kgryte
Copy link
Member

kgryte commented Jun 24, 2020

@JithinKS97 Awesome! Will review shortly!

@kgryte kgryte self-requested a review June 24, 2020 02:21
@kgryte
Copy link
Member

kgryte commented Jun 24, 2020

@JithinKS97 Looks good! The only questions I have are:

  1. The Julia REQUIRE file for generating test fixtures still says 0.5. Can you update this? And what version of JSON was necessary to support the Julia version?
  2. Did you have to refactor the Julia benchmarks file? If so, would you be able to update the REQUIRE file there? This will probably entail updating the BenchmarkTools dependency.

Thanks!

@JithinKS97
Copy link
Contributor Author

JithinKS97 commented Jun 25, 2020

@kgryte I've updated the REQUIRE in test It is now

julia 1.4.2
JSON 0.21.0

In the benchmark, I had to change the @printf function to print to make it run with Julia 1.4.2. The REQUIRE file in bench mark now is

julia 1.4.2
BenchmarkTools 0.5.0

Also when I ran c benchmark file, I got the warning

benchmark.c: In function ‘main’:
benchmark.c:124:9: warning: implicit declaration of function ‘time’ [-Wimplicit-function-declaration]
  srand( time( NULL ) );

Which got removed when I included time.h in header. Can I keep it that way?

@kgryte
Copy link
Member

kgryte commented Jun 26, 2020

@JithinKS97 Re: time.h. I am surprised this bug has taken so long to discover. We've been missing that header in all the C benchmark files. My guess is that the C compilers we've used up until this point have automatically included it, similar to math.h.

In short, yes, keep the time.h include.

@kgryte
Copy link
Member

kgryte commented Jun 26, 2020

@JithinKS97 Thanks for updating the REQUIRE files! LGTM.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@JithinKS97 Looks good to me. Merging! Thanks for working on this!

@kgryte kgryte merged commit 5b16f4d into stdlib-js:develop Jun 26, 2020
@JithinKS97
Copy link
Contributor Author

Thanks very much

@kgryte
Copy link
Member

kgryte commented Jun 26, 2020

@JithinKS97 For reference, I did a final tidy in this commit a9a6661 to match project conventions (style, etc), fix some "bugs" on our end (erroneous info in the package.json), and test/benchmark behavior (testing +-inf, adjusting value ranges to ensure better "branch coverage" in the underlying implementation, etc). May be useful to review when working on the other functions.

@JithinKS97
Copy link
Contributor Author

@kgryte Noted that 👍

@Planeshifter Planeshifter removed the Needs Review A pull request which needs code review. label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Issue or pull request for adding a new feature. 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 inverse/arc cotangent (acot)

3 participants