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

Riemann Zeta Function Implemented #2950

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

Bobingstern
Copy link
Contributor

I implemented the original version of this issue without the Riemann Siegel Formulation for the critical strip. It includes the analytic continuation using Riemann's Functional Equation. All tests are passing with Real, Complex and Irrational numbers. Please let me know if I need to add or change anything.

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

Wow, this looks good, thanks for working out this PR!

I made a couple of inline comments, can you have a look at those?

src/function/special/riemannZeta.js Outdated Show resolved Hide resolved
src/function/special/riemannZeta.js Outdated Show resolved Hide resolved
src/function/special/riemannZeta.js Outdated Show resolved Hide resolved
@@ -0,0 +1,87 @@
import { factory } from '../../utils/factory.js'

const name = 'riemannZeta'
Copy link
Owner

Choose a reason for hiding this comment

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

In Matlab and Mathematica for example you can use zeta. I see in your unit tests you prefer this short name zeta too. Is it an idea to just name the function zeta instead of riemannZeta?

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 named it riemannZeta since there are a bunch of zeta functions out there that I didn't want to confuse with such as the Hurwitz Zeta function. Since the Riemann zeta is the most well known of the zeta functions I think it's okay to rename it to zeta and if subsequent zeta functions are implement to name them hurwitzZeta etc

Copy link
Owner

Choose a reason for hiding this comment

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

It's a good point to make sure there will be no confusion. As far as I can see, Matlab uses zeta, Mathematica allows both zeta and riemannZeta, and Python combines both Riemann and and Hurwitz in a single function zeta, defaulting to Riemann (I personally would prefer to have two separate functions).

I'm OK with either one, what do you think is best?

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 think zeta is more concise and most people will think of riemannZeta when looking at zeta. I have no idea how to implement a fast algorithm for the Hurwitz zeta function but I agree making them separate makes the most sense. Let's switch to just zeta

Copy link
Owner

Choose a reason for hiding this comment

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

👍 sounds good

Copy link
Owner

Choose a reason for hiding this comment

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

Still planning on renaming the function and files zeta?

src/function/special/riemannZeta.js Outdated Show resolved Hide resolved
@Bobingstern
Copy link
Contributor Author

I pushed out a new one with Big Number support and all of the changes listed above. All tests are passing and docs have been added too.

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

The BigNumber support is awesome, thanks 😎 . I made a couple more inline comments, can you have a look at those?

* math.zeta(math.i) // returns 0.0033002..., -0.4181554491...i
*
*
* @param {number | Complex | BigNumber} s A real or complex number
Copy link
Owner

Choose a reason for hiding this comment

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

What would be a more informative description for the property s than "A real or complex number"? (missing BigNumber right now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed now in the newest commits

Copy link
Owner

Choose a reason for hiding this comment

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

😂 well it's technically OK now. What I meant is that the description is not really a description, it only tells the type of s, but that is already documented.

It's like when you ask me "what do you have there?" and my answer is "steel and plastic". That can be technically correct. But a more helpful answer would be "a screwdriver". Explaining what kind of thing it is or what meaning it has.

Not a big deal though. I can't come up with something meaningful. Maybe "The numeric input for the zeta function" or so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see what you are saying, sorry for the misunderstanding. I think I can come up something more descriptive

src/function/special/riemannZeta.js Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,87 @@
import { factory } from '../../utils/factory.js'

const name = 'riemannZeta'
Copy link
Owner

Choose a reason for hiding this comment

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

Still planning on renaming the function and files zeta?

@Bobingstern
Copy link
Contributor Author

For some reason I cant comment on the review. I think leaving the file names as riemannZeta and having the actual name to be just zeta would be better in the case of future versions implementing other zeta functions. If this is inconsistent to the other function then I can certainly change it. Also, I think you may have reviewed an older commit for the embedded docs since I have added those. The types have been updated too. The BigNumber aliases can be removed and reuse code if you think that is a better idea than duplicating the code for BigNumbers. I'm still fairly new to PRs so I apologise if I did something wrong with updating/fixing my code with GitHub

@josdejong
Copy link
Owner

Thanks for all the updates. I think we're about there. I made two more small remarks just to make sure we're on the same page.

For some reason I cant comment on the review.

hm that is odd. Maybe I wrongly created a second review with the second round of feedback, closing the first or so.

I think leaving the file names as riemannZeta and having the actual name to be just zeta would be better in the case of future versions implementing other zeta functions. If this is inconsistent to the other function then I can certainly change it.

I strongly prefer to keep the name of the actual function consistent with the names of the source files. I understand your reasoning but I think it will lead to confusion about whether riemannZeta is the same as zeta or not if they are mixed. Other future versions of zeta will simply have to pick a other, unique name like hurwitzZeta.

@Bobingstern
Copy link
Contributor Author

Alright that makes sense to me, I'll rename the source files and add the changes you requests possibly over the weekend

if (x > -(n - 1) / 2) { return fBigNumber(x, n) }

// Function Equation for reflection to x < 1
let c = multiply(pow(2, x), pow(BigNumber(Math.PI), subtract(x, 1)))
Copy link
Owner

Choose a reason for hiding this comment

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

This will not result in a true BigNumber version of pi. I think you can simply import pi instead? Or use the function createBigNumberPi?

@josdejong
Copy link
Owner

I see one unit test fails: https://github.com/josdejong/mathjs/actions/runs/5057022868/jobs/9149909403?pr=2950

Error: Required dependency "BigNumber" missing for factory "zeta" (suffix: Number)

I guess that is because in the number-only implementation of mathjs factoriesNumber.js, BigNumber does not exist. This can be solved in two ways:

  1. Make the dependency optional, like '?BigNumber' and add a check before using it to see whether it is defined.
  2. Rewrite the code so you don't need the BigNumber import: you can make zero by multiplying a variable from which you know it is a BigNumber by zero. You can create a BigNumber n by doing zero.add(n) where zero is a BigNumber, etc.

@Bobingstern
Copy link
Contributor Author

How would I create a BigNumber of zero if I can't import the BigNumber though? Or is there a universal zero variable?

@josdejong
Copy link
Owner

// given that x is a BigNumber:
const zero = x.times(0)
const bn = zero.plus(n)

@Bobingstern
Copy link
Contributor Author

I understand that, but how would I create x in the first place? I'm quite new to this so I apologize if this is a basic question

@Bobingstern
Copy link
Contributor Author

Ok, I renamed all the files to zeta, updated the ts types, and I added a more descriptive explanation for s. I just need your help fixing the BigNumber in build-test.

return NaN
}
// 15 decimals of accuracy
const n = 20
Copy link
Owner

Choose a reason for hiding this comment

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

by default, BigNumbers are configured to have a precision of 64 digits. Can we utilize config.precision here?

}
// 15 decimals of accuracy
const n = 20
if (x > -(n - 1) / 2) { return fBigNumber(x, n) }
Copy link
Owner

Choose a reason for hiding this comment

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

There are still a few places where you implicitly convert a BigNumber into anumber, like x === 0 and x > -(n - 1) / 2. I think they cannot hurt in practice since n is small and integer, but it would be neat to use equal(x, 0) and larger(x, -(n - 1) / 2) I think.

@josdejong
Copy link
Owner

Ok, I renamed all the files to zeta, updated the ts types, and I added a more descriptive explanation for s. I just need your help fixing the BigNumber in build-test.

Thanks for the updates. No problem at all. Getting BigNumbers right requires some effort: we need to be careful not accidentally throwing away precision by mixing numbers into the equation, and also we need to be careful to respect the configured precision, and use pi with the right precision etc.

Zooming a bit out I see we basically have two implementations: for real numbers and for complex numbers. The real number implementation is currently named zetaBigNumber and still a bit hard coded for BigNumbers, but we can make it a really generic function that accepts both number and BigNumber. It may be good to put the real number implementation in a separate file (for internal use), and use that in the zeta implementation. In the number-only version of mathjs (factoriesNumber.js) we could refer to this generic implementation since it works fine with just numbers. Does that make sense?

If you want we can make this PR a joint effort, I can do this last step of refining the BigNumber implementation if you like.

@Bobingstern
Copy link
Contributor Author

That would be fine with me! Do I need to change anything in order for you to modify the code in the pr?

@josdejong
Copy link
Owner

OK let's do that, I'll create a new feature branch from yours and work out the last details of the BigNumber implementation. I'll keep you posted.

@josdejong josdejong mentioned this pull request Jun 20, 2023
3 tasks
josdejong added a commit that referenced this pull request Aug 23, 2023
* Riemann Zeta Function

* Big Number zeta and added docs

* Original algorithm paper credited

* Update index.d.ts

* Update riemannZeta.js

* Update index.d.ts

* Renamed files to reflect zeta

* chore: make all the tests pass

* chore: refactor `zeta` (WIP)

* chore: reuse the validation logic of both number and BigNumber

* fix: type definitions of `zeta`

* fix: test the accuracy with numbers and BigNumbers (WIP)

* chore: make linter happy

* docs: fix example outputs

* docs: update history

* docs: update history

* docs: describe the limited precision of `zeta`

---------

Co-authored-by: BuildTools <[email protected]>
Co-authored-by: Anik Patel <[email protected]>
@josdejong
Copy link
Owner

Closing in favor of #2975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants