Issue50 #51
Issue50 #51
Conversation
…bort when on the separatrix
…onents are returned in association.
Kernel/FourVelocity.m
Outdated
| (*Circular, Equatorial*) | ||
|
|
||
|
|
||
| KerrGeoVelocityMino[(0|0.),p_,(0|0.),x_,initPhases_ ]:= Module[{En,L,Q,r,z,r1,r2,r3,r4,kr,zp,zm,kz, \[CapitalUpsilon]r, \[CapitalUpsilon]z, |
There was a problem hiding this comment.
The use of (0|0.) misses the case where a or e are non-machine precision zero's (e.g. 0.``32) I suggest the following instead
KerrGeoVelocityMino[a_,p_,e_,x_,initPhases_ ] /; a==0 && e==0There was a problem hiding this comment.
I see in the rest of the package ?PossibleZeroQ is used. This should do the same thing, but I also think it's better style to be consistent throughout the package and use
KerrGeoVelocityMino[a_?PossibleZeroQ,p_,e_?PossibleZeroQ,x_,initPhases_ ] := ....There was a problem hiding this comment.
PossibleZeroQ could be used here, although note the slightly different. PossibleZeroQ prints a message and returns True if a number might be zero but it's not sure, whereas the version with Equal will not return True in that case, which is equivalent to returning False for the purposes of the function definition. There are also circumstances where PossibleZeroQ will get it wrong (see the docs for example). These are all very much corner cases, so I think either PossibleZeroQ or Equal would be fine.
| kr = ((r1-r2)(r3-r4))/((r1-r3)(r2-r4)); | ||
|
|
||
| kz = a^2 (1-En^2) zm^2/zp^2; |
There was a problem hiding this comment.
Can these be removed now? These are the lines that lead to divide-by-zero at the separatrix.
There was a problem hiding this comment.
Do you mean the expressions for kr and kz?
These aren't singular at the separatrix. kz is finite and kr goes to 1. The problem is that EllipticK[kr] returns complex infinity when kr = 1 and that shows up in the frequencies, the analytic solutions for the coordinates and the four-velocities, with no obvious way (to me at least) for taking a finite limit exactly on the separatrix for generic orbits.
There was a problem hiding this comment.
When there is a double root, you get special solutions. These are currently not explicitly implemented in the package (except circular orbits). We should add a check for r2==r3 and produce an error message.
There was a problem hiding this comment.
Yes, I'm suggesting to remove the definitions of kr and kz. The problem with these is that in the case of repeated roots (eg. at the ISCO) they involve 0/0 and this causes problems. I had thought kr and kz are no longer used in this function, in which case we could remove them to avoid warning messages. Unfortunately I now see they are used further down. Could we define functions for these quantities also and then remove kr and kz here? The special handling of repeated roots can then be done in KerrGeoMinoFrequencyr and the other added functions.
|
|
||
| (*Other Roots*) | ||
| r3 = 1/(1-En^2) - (r1 + r2)/2 + Sqrt[(-(1/(1-En^2)) + (r1 + r2)/2 )^2 - (a^2 Q)/(r1 r2 (1 - En^2))]; | ||
| r4 = (a^2 Q)/(r1 r2 r3 (1-En^2)); | ||
| kr = ((r1-r2)(r3-r4))/((r1-r3)(r2-r4)); |
There was a problem hiding this comment.
We need the expression for kr in the function definition on line 198, so we can't just get rid of it.
There was a problem hiding this comment.
This is the same issue as above.
Partially fixes Issue #50
New features: