-
Notifications
You must be signed in to change notification settings - Fork 69
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
Rocket kick #1019
Rocket kick #1019
Conversation
Reverting to draft while I update the documentation. |
I couldn't quite work out where you rotate the frame so that the z axis becomes the rocket direction. It should come somewhere before the B and D support vectors are rotated. |
At the start of any SN, we consider the current z-axis aligned with the orbital AM. The rotation in lines 1203, 1419, 1440, 1441, and 1442 only matters at the end of the second SN calculation, when we rotate into the previous SN frame to correctly add the velocity vectors. If the rocket-kick angles are not set by the user, the kick is by default aligned to the z-axis. Does that answer your question? |
Ok, I think I understand what is going on now. |
I've made some of the changes toward fixing the AM vector, but I realized I might have gotten some of the units wrong elsewhere in the code (the units for orbital energy are a bit weird, I think). So I'm reverting this to draft for now, but feel free to look at the changes to see if they match expectations. Also, the derivation in the notes has changed, ya? It no longer has the B and D vectors. I think it doesn't make sense for me to use them in the code then, since it will confuse people who are using your paper as a reference. So I'll have to fix that up when I have a spare moment. |
Sorry, I did change the notation but the support vectors still exist. Instead of B and D, now they're called h_+ and h_-. |
Latest updates fix the definition of the normalized AM vector, the variables defined in the Hirai+ paper, and the mismatched units. Should be ready for re-review. |
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.
Looks good to me. It would be nice to later add a unit test for the vector rotations, etc.
Also, in a future iteration we should add in a check for cases where the rocket kick causes the orbit to reach e=1. At the moment, it will easily oscillate past e=1 back to e<1, but in reality the binary should experience a merger if it gets too close to e=1.
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.
Reviewing - placeholder - will get this done today
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.
Thanks @reinhold-willcox . Just a few minor things:
-
1. Since you've added new options, you should create a new default yaml file.
-
2. Although not strictly necessary, you should add the new options to the yaml template in yaml.h (COMPAS will create a yaml file with the new options, but if you don't put them in the template (in yaml.h), they'll just appear at the end of the file rather than in one of the sections in the file)
-
3. In line 773 in Options.h, change "1" to "2" in the comment. I think it would be better to use "primary" and "secondary" in the comments rather than "1" and "2" similar to the comments for "initial-mass-1" and "initial-mass-2" on lines 698 & 699 (the variables names already have "1" and "2" in them - "primary" and "secondary" describes what they are for better than just "1" and "2").
-
4. Remove the underscore in getters
RocketKick_Phi1()
,RocketKick_Phi2()
,RocketKick_Theta1()
, andRocketKick_Theta2()
- our convention is camelcase, not snakecase, and they're not consistent withRocketKickMagnitude1()
andRocketKickMagnitude2()
. (The SN_* getters get a reprieve because the "SN" is an abbreviation - if they were "Supernova*" instead of "SN_*" then they wouldn't need the "_"). -
5. You have the fallback parameter "true" for getters
RocketKickMagnitude1()
andRocketKickMagnitude2()
, but "false" forRocketKick_Phi1()
,RocketKick_Phi2()
,RocketKick_Theta1()
, andRocketKick_Theta2()
- is that a deliberate choice? If so, maybe we should document that somewhere - it's not consistent with other options. In fact, the only other options that have fallback set "false" are the SN_* getters, and I wonder if that was deliberate? Maybe we should revisit those. -
6. Line 1486 in
Options.cpp
- change "primary" to "secondary". -
7. You have a lot of blank lines around the new functions in
Vector3d.cpp
- did you mean to remove those? -
8. The new functions in
Vector3d.cpp
don't have the function signature in the description of the function (consistent with other function descriptions). -
9. Version number should increment to "02.42.00" rather than "02.41.05" - this is new minor functionality rather than a defect repair, so the middle digits should increment (major.minor.defect).
-
10. Documentation - in
program-options-list-defaults.rst
, the descriptions of--rocket-kick-magnitude-1
and--rocket-kick-magnitude-2
don't mention primary/secondary. -
11. Documentation - I think you should announce the new functionality (just briefly) on the "What's New" page (
whats-new.rst
)
…rect compas case and clarifying primary vs secondary star
@jeffriley I think I've addressed all your points (I added checkboxes to your message). I agree we may want to revisit the |
@reinhold-willcox Changes look good, thanks - but I can't see the new default yaml file. You updated yaml.h, but did you create the new file (--create-yaml-file)? Did you forget to push it, or am I just not seeing it? |
@jeffriley My mistake, I had thought it would happen automatically when pushed up to github. It's fixed now. |
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.
All good - thanks @reinhold-willcox
Functionality for the Hirai pulsar rocket kick.
New options are "--rocket-kick-magnitude-X", "--rocket-kick-phi-X", and "--rocket-kick-theta-X" where X is either 1 or 2 (rockets are not implemented for single stars).
As described in the help text, magnitude is in km/s, theta is a polar angle (defined as 0 when parallel to the orbital AM), and phi is a planar angle. Assumes the newborn pulsar spin axis is aligned with the pre-SN orbit.