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

Add Number of Orbit and Fraction of Orbit Triggers #6009

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

AlexCarpenter46
Copy link
Contributor

@AlexCarpenter46 AlexCarpenter46 commented May 20, 2024

Proposed changes

Adds a trigger to do something the user specifies after a certain number of orbits. Addresses #5938

If you want to try either of these triggers out for something like eccentricity control or Observations at certain points in. your orbit, add this to the EventsAndTriggers sections in your yaml.

  - Trigger:
      NumberOfOrbits:
        Value: 1.0
    Events:
      - Completion
  - Trigger:
      FractionOfOrbit:
        Value: 0.25
    Events:
      - ObservationAhA
      - ObservationAhB

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

(i->second.get()));
if (rot_f_of_t != nullptr) {
const double calculated_orbits =
rot_f_of_t->angle_func(time)[0][2] / (2.0 * M_PI);
Copy link
Member

@nilsdeppe nilsdeppe May 21, 2024

Choose a reason for hiding this comment

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

Is this assuming orbits around a specific plane? We can, and likely will, run into issues with precession at some point, so what we need to make sure is to use something that works with precession, even if the orbit becomes say in the y-z plane and phi=const.

Edit: I think for quaternion use we'd need to integrate the arc length. SpEC assumes a point on the x-axis in the grid frame, maps that to the inertial frame and computes the angle between the current and previously triggered point. We also should have some way of checking that the map isn't stationary. We had a bug for months in SpEC where the ringdown GWs weren't observed because the input files were using FractionOfOrbit. This can probably be done with something like checking that the derivatives are close to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this assumes the binary orbits in the x-y plane like what's currently done in /tools/Status/ExecutableStatus/EvolveGhBinaryBlackHole.py. Also I'm not sure if I'm following completely, but then we'll need to track a point throughout the evolution integrating its arc length and comparing that to its original radius to find the angle? Also I tried to account for stationary maps with the Error if the rotation control system isn't active, but I guess that requires the user to change the control system section, so I'll change that to also check if omega is close to 0.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that'll be a problem once we start having precessing runs so we need some way to guard against that. Presumably checking that the other angles are zero and ERRORing if they aren't would suffice. This sounds like we already have a bug in the Status executable.

In terms of arc length: I don't have any specific idea, but you need a 3d way of tracking this. What SpEC does is it uses a point on the x-axis in the grid frame, then measures the angle between the inertial location the last time the trigger ran and now. If that angle is larger than the fraction of an orbit, it triggers and updates the stored angle. This is fine if you are observing at least 4 times an orbit and handles precession. If you want to observe less often than that, you need a way of defining an orbit for a orbits that don't close. You could do something like arc length at unit radius reach 2pi. I don't know if SpECTRE's triggers can be stateful, but that's essentially what you need to track this correctly. If we don't have stateful triggers, you could require the other angles to be close to zero.

Right, though in the ringdown we slowly decrease the rotation rate until it hits zero. This means the rotation map is present, it's just no longer changing, which causes the trigger to never trigger and means that using the trigger is an error. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I've been confusing 2 things here and need to clarify that.

  1. A trigger that triggers every fraction of an orbit. This is doable and what I described in my last comment. SpEC uses this for triggering GW observations during the inspiral. I think we should do the same and most of the code here is needed for that.
  2. A trigger that stops after N orbits. I don't know how to do this and it's sort of ill-defined. SpEC just sets a final time to measure the eccentricity. I think we should just do that. I'm not sure why we would deviate here from SpEC or who advised you to do so, but what SpEC does has worked well for a long time and I've never heard of any discussions of changing when we terminate for eccentricity reduction.

With that said, I'm not sure what the path for this PR is. It seems like it's not something I think we should do since it deviates from SpEC and doesn't trivially generalize to precessing systems. However, basically everything you have is needed for triggering a fraction of an orbit, and that I think is useful and we do need! We should probably discuss this in a call with whomever advised you to implement this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense! I think changing it to the fraction of an orbit sounds good if nobody else is working on that. Nobody really advised me to implement this or do it this way, I just saw the issue and asked @knelli2 and @nilsvu if I could take a crack at it :). I would definitely like to discuss this more in a call. I won't be available tomorrow, but we could talk about this at the technical call if that works.

Copy link
Member

Choose a reason for hiding this comment

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

The technical call works for me! I'm trying to figure out what the angle you're using actually means. If it's the angle about the z-axis it would be problematic, but since we use quaternions in the control system it might be the angle about the axis of rotation, in which case it might actually be totally fine. I'll have to ask Kyle & Mark about that tomorrow :)

(i->second.get()));
if (rot_f_of_t != nullptr) {
const double calculated_orbits =
rot_f_of_t->angle_func(time)[0][2] / (2.0 * M_PI);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I've been confusing 2 things here and need to clarify that.

  1. A trigger that triggers every fraction of an orbit. This is doable and what I described in my last comment. SpEC uses this for triggering GW observations during the inspiral. I think we should do the same and most of the code here is needed for that.
  2. A trigger that stops after N orbits. I don't know how to do this and it's sort of ill-defined. SpEC just sets a final time to measure the eccentricity. I think we should just do that. I'm not sure why we would deviate here from SpEC or who advised you to do so, but what SpEC does has worked well for a long time and I've never heard of any discussions of changing when we terminate for eccentricity reduction.

With that said, I'm not sure what the path for this PR is. It seems like it's not something I think we should do since it deviates from SpEC and doesn't trivially generalize to precessing systems. However, basically everything you have is needed for triggering a fraction of an orbit, and that I think is useful and we do need! We should probably discuss this in a call with whomever advised you to implement this.

@AlexCarpenter46 AlexCarpenter46 added the in progress Don't review, used for sharing code and getting feedback label Jun 9, 2024
@AlexCarpenter46 AlexCarpenter46 changed the title Add Number of Orbits Trigger Add Number of Orbit and Fraction of Orbit Triggers Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Don't review, used for sharing code and getting feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants