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

Plunging Geodesics Kerr pull request #44

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

ConorDyson
Copy link

Pull request for plunging geodesics in Kerr, Analytic implementation in terms of Mino time, solves for generic plunges given {a,e,L,Q} and can specialise to plunges from the ISSO for parameterizations {a,r_{ISSO}} or {a,\theta_{inc}}. Units test have also been included, and packaged sits on top of existing ones. The only change to existing packages should be the addition of the line "KerrGeodesicsKerrGeoPlunge" in the KerrGeodesics.m run file.

ConorDyson and others added 30 commits November 23, 2022 15:35
Version1 of plunge package working for ISSO, Complex and Real Roots - ISSO currently in separate package to Generic solution file, neither implemented in the original GeoOrbit package yet.
TestPlungesProducingPaperPlots
Package working ISSO for radial param and inclined param. Commit also contain test file with flow and orbital plots, need to include initial conditions and return mino time crossing horizon still
Small bug fixed, now works for ICs. Noticed generic coord tIme solution still is not fully analytic Arctan term is causing issues needs to be massaged to an analytic form.
…canonical keyword Automatic for empty input (instead of "Nan")
Real Root Code has some issues, added all crossing times and fixed format of complex roots file, also added all initial conditions properly.
All fixes seem to be working, just need to run a script to test all possible cases i.e. code works when it should and gives correct error messages when it shouldn't
This reverts commit 8d1284f.
L now works for all allowed r_I and ISSO works for equatorial case now.
MIno time and other outputs improved to correct form
Tests provide in proper Mathematica Test File now
Moved test file to test folder and removed unnecessary additional files
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.

There appear to be multiple PlungesTest files: one in Kernel and two inside Tests/. Can these be merged into a single file? Also, can they be converted to wlt files for use with the automated testing system?

Further comments are also listed below.

Kernel/KerrGeodesics.m Outdated Show resolved Hide resolved
Kernel/KerrGeoOrbit.m Show resolved Hide resolved
"KerrGeodesics`FourVelocity`"}];


KerrGeoPlunge::usage = "Takes either KerrGeoPlunge[a, Generic, En ,\[ScriptCapitalL],\[ScriptCapitalQ]] or KerrGeoPlunge[a, ISSO , RI] for generic Plunges or ISSO plunges respectively and returns a KerrGeoPlungeFunction[..] which stores the orbital trajectory and parameters. Here the ISSO plunges are paramaterised in terms of the choice of the radius of the ISSO for a given a there are range of allowed RI's which correspond to differeing inclinations in the prograde and retrograde directions, if an RI outside this range is given the used is provided with the range of allowed values to re-run the funciton.";
Copy link
Member

Choose a reason for hiding this comment

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

We need to think carefully about this interface. Do we want separate functions for ISSO plunges? Or can everything be inferred automatically and unambiguously from context?

Copy link
Author

Choose a reason for hiding this comment

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

I've reworded this section slightly; the usage is as follows.

For generic plunges you provide either KerrGeoPlunges[a, {e,L,Q}] or KerrGeoPlunges[a, {e,L,Q}, {t0,to,theta0,phi0}] if you want to include initial conditions

For ISSO, one provides either KerrGeoPlunges[a, "ISSORadialParam", RI] or KerrGeoPlunges[a, "ISSOIncParam", theta_Inc] with IC's working the same as the generic case as a final option.

The package defaults to Generic unless the user explicitly inputs an ISSO option which I think should be sufficiently unambiguous but I'm happy to consider alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

I agree the main issue will be deciding on the interface. This should probably go along with the discussion related to issue #37 and associated pull request #38. We were thinking of having default input parameters as (a,p,e,x) but then allowing associations to be passed if the user wishes to use alternative parameterizations.

We also need to decide if we want KerrGeoPlunges or to wrap this within KerrGeoOrbit

Copy link
Author

@ConorDyson ConorDyson Feb 15, 2023

Choose a reason for hiding this comment

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

I'm not sure if parameterization by (a,p,e,x) is overly suitable for plunges. @MvdMeent also mentions in #37 how this parameterization breaks down for plunges. If we wanted a parameterization that works well for all classes of bound geodesics (including plunges), perhaps parameterizing by initial position and velocity, as Vojtech mentions in #37 might be best? Theres probably other reasons for why this might not be a good choice that I'm unaware of though.

If we do decide to wrap KerrGeoPlunges within KerrGeoOrbit, we do have to note the fact that for a large part of the parameter space, a given (a,E,L,Q) gives two solutions, one bound in the regular sense, and another "deeply bound"(plunging). So if we do this, we need to think about if we want the user to have to specify if they're looking for a plunging or bound geodesic when they run the code. Perhaps this isn't an issue if we can parameterize the plunges by (a,p,e,x) or initial positions and velocities, though?

I do think the ISSO case parameterization should be kept largely the same as it currently is however, because the nice thing about those solutions is being able to fully parameterize everything by a single choice of R_{ISSO}.

@nielsw2
Copy link
Member

nielsw2 commented Feb 14, 2023

I noticed that the a=0 case does not seem to work. e.g.,

KerrGeoPlunge[0, {0.9, 0.7, 0.4}] gives a bunch of errors.

@ConorDyson
Copy link
Author

The package currently doesn't work for the a=1 or a=0 case; I've added an error message that kills the run if either of these choices is made I'll see if the functions can be easily adapted to work in the a=0 case I'm pretty certain there is no easy way to make it work in the extremal case but ill check also.

Working for a=0 still gives error message for a=1. a=0 case has been extended through the manifold singularity but is still continuous and satisfies the EOM so seems to be working correctly.
closed section opened
@ConorDyson
Copy link
Author

Both the generic case and ISSO now work for a=0.

@nielsw2 nielsw2 added this to the Version 1.0.0 milestone Mar 16, 2023
@nielsw2 nielsw2 self-assigned this May 12, 2023
@nielsw2
Copy link
Member

nielsw2 commented Jul 8, 2023

Just so we don't forget. Following a meeting on the 24th of Feb. we decided on the following API:

KerrGeoOrbit[a,"\[ScriptCapitalE]\[ScriptCapitalL]\[ScriptCapitalQ]"->{0.9,3,1.2},"InitialPhases"->{1,2,4}]
KerrGeoOrbit[a,"\[ScriptCapitalE]\[ScriptCapitalL]\[ScriptCapitalQ]"->{0.9,3,1.2},"OrbitType"->"Plunge"]
KerrGeoOrbit[a,"Subscript[r, ISSO]"->5,"OrbitType"->"Plunge"]
KerrGeoOrbit[a,"InitialPosition"->{1,2,3,4},"FourVelocity"->{1,5,6,7}]

This is the main change needed for before merging this pull request and #38

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.

4 participants