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

Added PN stuff (not tested) #53

Merged
merged 18 commits into from
Jan 28, 2025
Merged

Added PN stuff (not tested) #53

merged 18 commits into from
Jan 28, 2025

Conversation

jakobneef
Copy link
Collaborator

This adds support for PN series of TeukolskyRadialPN and TeukolskyPointParticleModePN

@barrywardell
Copy link
Member

I'm having trouble getting TeukolskyPointParticleModePN. The output includes things like SpinWeightedSpheroidalHarmonicS[-2, 2, 2, 2 a \[CapitalOmega]Kerr][\[Pi]/2, 0]. Is there an updated to SpinWeightedSpheroidalHarmonics required to get it to work?

@barrywardell
Copy link
Member

Also a few more comments:

  • There is a new symbol \[CapiatlOmega]Kerr exposed by TeukolskyPointParticleModePN. Is there a better, non-hacky way to achieve this? Perhaps write \[CapiatlOmega]Kerr explicitly in terms of p and a?
  • When the amplituudes are not computed, we should probably return them as Missing["NotComputed"]
  • The documentation refers to setting options to "True" (with quotes). Should this be True (without quotes)?
  • When returning a RadialFunction from a mode, should we just return a TeukolskyRadialFunctionPN? This would avoid the aesthetic problems mentioned in the documentation.
  • There are some typos in the documentation, eg. a link with just ::

@jakobneef
Copy link
Collaborator Author

  • The issue with the SpinWeightedSpheroidalHarmonicS and the \[CapiatlOmega]Kerr are related. It would be possible to express \[CapiatlOmega]Kerr as 1\(r0^(3\2)+a), however to be consistent we would have to PN expand that expression, since r0 carries powers of eta. To be consistent then as well we would have to expand the SpinWeightedSpheroidalHarmonicS and it's derivatives as they depend on OmegaKerr. At the moment we opted against this to allow the user to get the coefficients of the S, S' and S''. To be honest I am unhappy with both possible options and very open to input on that question.
  • I'll fix that.
  • I'll fix that.
  • I'm not sure I understand that point. Maybe we can discuss it tomorrow?
  • I'll fix that

Thanks for the testing! I'll work in the fixes tomorrow.

@barrywardell
Copy link
Member

It looks like most things are now resolved. Are there any outstanding issues?

@jakobneef
Copy link
Collaborator Author

Only thing is I haven't updated the documentation to the changes I made today. I doubt it will take me longer than a day.

@barrywardell
Copy link
Member

I've updated the documentation, so I think that's everything and the PR is ready to be merged.

Copy link
Member

@barrywardell barrywardell left a comment

Choose a reason for hiding this comment

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

All comments have been resolved

@barrywardell barrywardell merged commit a0baca4 into master Jan 28, 2025
1 check failed
@barrywardell barrywardell deleted the PN branch January 28, 2025 20:16
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