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

Make Dommaschk factorials more accurate #1278

Open
dpanici opened this issue Sep 27, 2024 · 5 comments
Open

Make Dommaschk factorials more accurate #1278

dpanici opened this issue Sep 27, 2024 · 5 comments
Assignees
Labels
enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc

Comments

@dpanici
Copy link
Collaborator

dpanici commented Sep 27, 2024

n! / m!

n! = gamma(n+1)
gammaln(n+1) = log(n!)
exp(gammaln(n+1) - gammaln(m+1)) = n!/m!

@dpanici
Copy link
Collaborator Author

dpanici commented Sep 27, 2024

might fix some numerical issues I suspect are happening at higher resolutions in the dommaschk

@YigitElma
Copy link
Collaborator

I don't know about your numerical issue but I remember that using factorials is much more accurate than gammaln, since latter uses some approximation instead of actually calculating the factorials. For non-integral numbers, that is the only choice but if n and m are integers, I would recommend using factorials. This was used in one of the Zernike PRs and improved the accuracy substantially, probably #1037

@dpanici
Copy link
Collaborator Author

dpanici commented Sep 30, 2024

yea but they need to be differentiable unfortunately. However... since they are a basis, I think I could make some sort of transform matrix for them and pre-compute the basis... I should look into that instead of doing this (as I just checked and I already use gammaln actually)

@dpanici
Copy link
Collaborator Author

dpanici commented Sep 30, 2024

Actually I cannot do that since R,Z,phi are the coordiantes and so we never have a fixed grid in them. I can, however, pre-compute all the factorials and use math.factorial for that, since those are purely functions of the mode numbers of the basis... yea I can do that at initialiazaiton of the Dommaschk field or something.

make a self._constants or something that will get passed into the dommaschk_potential call that contains the various non-coordinate-dependent and non-coefficient-dependent parts of the potential (might though be a bit more complex, as they will need to likely be np arrays of the same length as the number of modes in the basis, since each different mode has a different arg to these constant parts)

@dpanici dpanici changed the title Replace fractions of factorials in Dommaschk with gammaln Make Dommaschk factorials more accurate Sep 30, 2024
@dpanici dpanici self-assigned this Sep 30, 2024
@dpanici dpanici added the enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc label Sep 30, 2024
@dpanici
Copy link
Collaborator Author

dpanici commented Sep 30, 2024

possibly rising factorial

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc
Projects
None yet
Development

No branches or pull requests

2 participants