Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Issue50 #51
base: master
Are you sure you want to change the base?
Issue50 #51
Changes from 4 commits
6e9d7bb
ab16e47
92e1100
1f6492a
29ddd77
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of (0|0.) misses the case where
a
ore
are non-machine precision zero's (e.g. 0.``32) I suggest the following insteadThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same issue as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if we could fix the KerrGeoMinoFrequency* functions to work in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but from my preliminary look at this, there doesn't seem to be an easy finite limit that can be taken. These frequencies may need to be completely re-derived for this case. Hence the next best thing of stopping the user from using the functions on the separatrix until these are known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there common sub-cases (e.g. ISCO) where we could make it work, but leave the generic case for another time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of unrelated whitespace changes here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why these all changed. I didn't manually add these in so it must have been automatic.