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

Parallel transport dev #32

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

Parallel transport dev #32

wants to merge 8 commits into from

Conversation

MvdMeent
Copy link
Collaborator

Several improvements:

  • Implementation of ParallelTransportFrame with "Phases" parametrization (i.e. frame parameterized by action angles)
  • Implementation of KerrGeoOrbit with "Phases" parametrization (analytic method only). (This breaks some of the usual functionality of KerrGeoOrbitFunction)
  • Expose MarckCarterTetrad and PrecessionPhase in ParallelTransportFrame output.
  • Expose Deltatr, Deltattheta Deltaphir Deltaphitheta in KerrGeoOrbitFunction (analytic method only)

May need further updates on:

  • Documentation.
  • Unit tests.

@MvdMeent MvdMeent requested a review from znasipak July 29, 2022 07:17
Copy link
Collaborator

@znasipak znasipak left a comment

Choose a reason for hiding this comment

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

Just reiterating my comments below: Print statements should changed to Messages, we should expand the functionality that @MvdMeent added to KerrGeoOrbitPhases to also work for the FastSpec method. We also need to add a key for initial phases. These will be needed to get the source integration correct. @znasipak can implement these changes. The ParallelTransport subpackage appears to work, but I have nothing to compare the results against. It would be better if Lisa Drummond or Viktor Skoupý reviewed this subpackage


assoc = Association[
"a" -> a,
"p" -> p,
"e" -> e,
"Inclination" -> x,
"Parametrization"->"Mino",
"Parametrization"->"Phases",
"Energy" -> En,
"AngularMomentum" -> L,
"CarterConstant" -> Q,
"ConstantsOfMotion" -> consts,
"RadialFrequency" -> \[CapitalUpsilon]r,
"PolarFrequency" -> \[CapitalUpsilon]\[Theta],
"AzimuthalFrequency" -> \[CapitalUpsilon]\[Phi],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a "TimeFrequency" key as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added it to the "Frequencies", but did not added a top level key, because I did not know what to best call it.

@@ -245,11 +297,12 @@
method = OptionValue["Method"];
param = OptionValue["Parametrization"];

If[param =!= "Mino", Print["Only Mino time parametrization has been implemented for parallel transport."];Return[];];
If[param =!= "Mino"&& param =!= "Phases", Print["Only Mino time parametrization has been implemented for parallel transport."];Return[];];
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should change these Print statements to Messages

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll take care of this (and other) changes to ParallelTransport.m

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -245,11 +297,12 @@
method = OptionValue["Method"];
param = OptionValue["Parametrization"];

If[param =!= "Mino", Print["Only Mino time parametrization has been implemented for parallel transport."];Return[];];
If[param =!= "Mino"&& param =!= "Phases", Print["Only Mino time parametrization has been implemented for parallel transport."];Return[];];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another instance of a Print statement that we probably want to change to a Message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -277,6 +330,7 @@


KerrParallelTransportFrameFunction[a_, p_, e_, x_, assoc_][\[Lambda]_/;StringQ[\[Lambda]] == False] := assoc["ParallelTransportedFrame"][\[Lambda]]
KerrParallelTransportFrameFunction[a_, p_, e_, x_, assoc_][\[Lambda]__] := assoc["ParallelTransportedFrame"][\[Lambda]]
KerrParallelTransportFrameFunction[a_, p_, e_, x_, assoc_][y_?StringQ] := assoc[y]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add Keys support to KerrParallelTransportFrameFunction, just as we have for KerrGeoOrbitFunction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

2) replace print statements with proper warning messages.
@barrywardell barrywardell mentioned this pull request Jul 29, 2022
znasipak added 2 commits July 29, 2022 14:11
…nded the TrajectoryOscillatory function association to the FastSpec method
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