Add support for tangent function #231

rhannequin · 2022-06-28T20:51:57Z

Associated issue: #81

Context

BigMath module supports sine (sin) and cosine (cos) trigonometric functions. It also supports arctangent (acan). But it doesn't support tangent, which is supported in Math module.

Solution

tan is related to sin and cos function such as (source):

tan(x) = sin(x) / cos(x)

This PR adds BigMath.tan(x, precision), dependent on existing BigMath.sin and BigMath.cos methods.

rhannequin · 2022-07-25T12:38:31Z

Hello @mrkn, anything I should do to this PR to improve it?
Please let me know if I missed a step to have this PR reviewed 🙏
Thank you.

mrkn

@rhannequin Thank you for your cooperation. I left some comments. Could you please check them?

mrkn · 2022-07-25T13:28:41Z

test/bigdecimal/test_bigmath.rb

+    assert_in_delta(0.0, tan(BigDecimal("0.0"), N))
+    assert_in_delta(0.0, tan(PI(N), N))
+    assert_in_delta(1.0, tan(PI(N) / 4, N))
+    assert_in_delta(Math.sqrt(3.0), tan(PI(N) / 3, N))


Use BigMath.sqrt(BigDecimal(3), N) instead of Math.sqrt(3.0).
Moreover, I guess we need to use assert_equal instead of assert_in_delta for all the assertions in this test method.

Note that I think we must replace assert_in_delta with assert_equal in all the other test methods, but they are out of this PR's target.

I want to check the 10*N-digit case in addition to the N-digit case for $\pi/3$. So could you add the following assertion?

Suggested change

assert_in_delta(Math.sqrt(3.0), tan(PI(N) / 3, N))

assert_in_delta(Math.sqrt(3.0), tan(PI(N) / 3, N))

assert_equal(sqrt(BigDecimal(3), 10*N), tan(PI(10*N) / 3, 10*N))

Hello @mrkn, thank you for your review 🙏

I used assert_in_delta for your suggestion because the definition of tan (sin / cos) makes the two methods' precision sum:

require "bigdecimal/math" BigMath.sin(BigDecimal("1"), 10) # => 0.84147098480789650665250231788602089836677454e0 BigMath.cos(BigDecimal("1"), 10) # => 0.540302305868139717400936607637646275593203e0 BigMath.sin(BigDecimal("1"), 10) / BigMath.cos(BigDecimal("1"), 10) # => 0.1557407724654902230506974799967261008769125258846034219578498192680719427656331763435628519e1

Using assert_equal in this case fails as the result has a lot more precision than N.

Do you think we should change the tan definition to handle precision differently?

Do you think we should change the tan definition to handle precision differently?

We must produce at least N precise digits for BigMath.tan(x, N), so the answer is yes.
Instead, I think we can use assert_in_delta with specifying BigDecimal("1e-#{N+1}") in the delta argument.

Hello @mrkn
Thank you again for reviewing this PR.

I had a look a other tests on the gem and I found that we use assert_in_delta in most of them, including cos and sin.
I understand that this may not be exactly what we're supposed to use for more accurate testing. Although, do you think we should first add this missing method (I'm also planning on adding acos and asin right after) the way the rest of the gem was implemented?

I'll be happy to help in the same time on updating gem to assert_in_delta in a way that is suitable for BigDecimal.
Please let me know what you think.

The reason why I'm sticking to a precise alternative of assert_in_delta is that your implementation of tan depends on sin and cos, and they are tested by assert_in_delta.

If tan and other new methods you plan won't depend on other methods tested by assert_in_delta, it can be merged.

Thank you for this explanation and the link to the work on test-unit, I see better why it is important to wait for a prior work.
I think it's better to stick to a definition of tan based on sin and cos for the simplicity of the gem (of course I'm all ears to counter arguments).

How can I help on your current PR on test-unit? This is a bit blurry to me but I guess I have to start somewhere.

Fixing assert_in_delta is almost done. After the new version of test-unit is released, I will update the master branch of this repository. And then, please rebase this branch and fix the test.

I think it's better to stick to a definition of tan based on sin and cos for the simplicity of the gem (of course I'm all ears to counter arguments).

If you can implement tan as a series that converges faster than sin and cos, using the series is better. If not, you can at least depend on only sin by applying basic trigonometric formula $\cos x = 1 - \sin^2 x$. Could you please take and compare benchmark results of both $\sin x / \cos x$ and $\sin x / (1 - \sin^2 x)$ to decide which is better?

Additionally, we can treat the case of $x = 0$ as the special case that just returns 0. And, we can limit the sin call only in the case of $x \in (-\pi/2, \pi/2)$ because tan is periodic.

mrkn · 2022-09-25T12:14:13Z

I'm working on supporting BigDecimal in assert_in_delta family assertions at test-unit/test-unit#218.

mrkn · 2022-10-08T02:22:55Z

I'm working on adding tests with higher precision at #238

rhannequin · 2022-10-15T20:21:36Z

@mrkn Hello, I'm back after a small break. Maybe I can help with #238?

mrkn · 2022-10-15T23:49:27Z

@rhannequin Although test-unit now supports BigDecimal in assert_in_delta family assertions, as tests of BigDecimal need to run also in Ruby's test-all, I have to let the test framework in Ruby's test-all support the new assert_in_delta family assertions.

Until finishing this work, could you try to rewrite tan implementation to depend on only sin?

rhannequin · 2022-10-30T13:46:57Z

Hello @mrkn, I hope you're doing great.

I did some research for a new algorithm for cos and arrived to the conclusion to just duplicate it, for now, so that tan only depends on sin.

I committed it a few minutes ago, what do you think?

mrkn · 2022-10-31T00:45:08Z

lib/bigdecimal/math.rb

+    end
+
+    sine / cosine
+  end


@rhannequin Could you please mention the origin such as the paper reference of this algorithm in the comment?
And, I want to know how this algorithm is better than just doing sine/(1 - sine**2). Could you explain?

@mrkn I don't believe tanθ is defined as sinθ / (1 - sin²θ), but should be sinθ / cosθ and cosθ is √(1 - sin²θ).

@nobu The current code was updated against my comment above. Please compare the commit timestamp and the comment timestamp.

rhannequin · 2024-02-16T09:44:54Z

@mrkn Hello, sorry for taking so long to move forward with this pull request.

My math background is not really advanced and I wasn't familiar with this definition of cosine based on sine.
I changed the implementation of tan to only depend on sin with this new definition, which is indeed much clearer and probably much faster.

Would you still like a link to a paper or an "official" definition for the documentation?

rhannequin · 2024-02-21T21:23:01Z

@hsbt I allow myself to ping you here as you seem to have been active on the project recently.
Please let me know if this PR needs to be reviewed by someone in particular, or if anything must be changed in the content.

mrkn · 2024-02-22T00:09:56Z

@rhannequin He’s recent work is only for leaving this library out of Ruby’s default gems. I think there are few people who can review new features of bigdecimal in the Ruby core team. Could you please wait until I can look into your new commits?

rhannequin · 2024-02-22T05:59:12Z

Absolutely, @mrkn. Sorry if my comment sounded pressing, I wasn't sure you had time for this at the moment. I'll wait now all the time needed.

mrkn · 2024-02-22T08:38:51Z

@rhannequin Thank you for your understanding and cooperation!

mrkn · 2024-09-09T04:35:38Z

Since bigdecimal is now a bundled gem, I think we only need to update the version of test-unit to ensure that the tests can verify the full digits.

mrkn requested changes Jul 25, 2022

View reviewed changes

rhannequin requested a review from mrkn July 28, 2022 07:56

mrkn reviewed Oct 31, 2022

View reviewed changes

rhannequin force-pushed the add_bigmath_tan branch from aa1e42a to 38bd414 Compare February 16, 2024 09:42

rhannequin requested a review from mrkn February 16, 2024 09:42

rhannequin added 3 commits September 9, 2024 13:47

Add support for tangent function

fc016a6

Apply review comments for tests

83c2723

Make tan depend only on sin

46dca76

mrkn force-pushed the add_bigmath_tan branch from 38bd414 to 46dca76 Compare September 9, 2024 04:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for tangent function #231

Add support for tangent function #231

rhannequin commented Jun 28, 2022

rhannequin commented Jul 25, 2022

mrkn left a comment •

edited

Loading

mrkn Jul 25, 2022

mrkn Jul 25, 2022

mrkn Jul 25, 2022 •

edited

Loading

rhannequin Jul 28, 2022 •

edited

Loading

mrkn Jul 28, 2022

rhannequin Sep 21, 2022

mrkn Sep 25, 2022

rhannequin Sep 26, 2022

mrkn Oct 1, 2022 •

edited

Loading

mrkn Oct 1, 2022 •

edited

Loading

mrkn commented Sep 25, 2022

mrkn commented Oct 8, 2022

rhannequin commented Oct 15, 2022

mrkn commented Oct 15, 2022

rhannequin commented Oct 30, 2022

mrkn Oct 31, 2022 •

edited

Loading

nobu Sep 9, 2024

mrkn Sep 9, 2024

rhannequin commented Feb 16, 2024

rhannequin commented Feb 21, 2024

mrkn commented Feb 22, 2024

rhannequin commented Feb 22, 2024

mrkn commented Feb 22, 2024

mrkn commented Sep 9, 2024

	assert_in_delta(Math.sqrt(3.0), tan(PI(N) / 3, N))
	assert_in_delta(Math.sqrt(3.0), tan(PI(N) / 3, N))
	assert_equal(sqrt(BigDecimal(3), 10N), tan(PI(10N) / 3, 10*N))

Add support for tangent function #231

Are you sure you want to change the base?

Add support for tangent function #231

Conversation

rhannequin commented Jun 28, 2022

Associated issue: #81

Context

Solution

rhannequin commented Jul 25, 2022

mrkn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkn Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

rhannequin Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrkn Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

mrkn Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

mrkn commented Sep 25, 2022

mrkn commented Oct 8, 2022

rhannequin commented Oct 15, 2022

mrkn commented Oct 15, 2022

rhannequin commented Oct 30, 2022

mrkn Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhannequin commented Feb 16, 2024

rhannequin commented Feb 21, 2024

mrkn commented Feb 22, 2024

rhannequin commented Feb 22, 2024

mrkn commented Feb 22, 2024

mrkn commented Sep 9, 2024

mrkn left a comment •

edited

Loading

mrkn Jul 25, 2022 •

edited

Loading

rhannequin Jul 28, 2022 •

edited

Loading

mrkn Oct 1, 2022 •

edited

Loading

mrkn Oct 1, 2022 •

edited

Loading

mrkn Oct 31, 2022 •

edited

Loading