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

Radial velocities #1638

Merged
merged 8 commits into from
Sep 14, 2023
Merged

Radial velocities #1638

merged 8 commits into from
Sep 14, 2023

Conversation

dlakaplan
Copy link
Contributor

@dlakaplan dlakaplan commented Sep 1, 2023

#374

Works for DD, DDS, BT, ELL1, ELL1H models. Returns the RV of the pulsar or companion.

@abhisrkckl
Copy link
Contributor

Works for DD and ELL1 models

Does it work for variations of these models like ell1h?

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Patch coverage: 87.50% and project coverage change: +0.02% 🎉

Comparison is base (25f38ca) 68.26% compared to head (cb8cdfd) 68.29%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1638      +/-   ##
==========================================
+ Coverage   68.26%   68.29%   +0.02%     
==========================================
  Files         100      100              
  Lines       23111    23119       +8     
  Branches     4010     4011       +1     
==========================================
+ Hits        15777    15789      +12     
+ Misses       6343     6341       -2     
+ Partials      991      989       -2     
Files Changed Coverage Δ
src/pint/models/timing_model.py 83.64% <87.50%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@dlakaplan
Copy link
Contributor Author

Works for DD and ELL1 models

Does it work for variations of these models like ell1h?

I haven't tried it yet - that's why this is still WIP.

@dlakaplan
Copy link
Contributor Author

(also needs tests)

@dlakaplan
Copy link
Contributor Author

I think this is probably ready, unless there are other tests.

@dlakaplan dlakaplan changed the title WIP: Radial velocities Radial velocities Sep 5, 2023
@dlakaplan dlakaplan added the awaiting review This PR needs someone to review it so it can be merged label Sep 5, 2023
Returns
-------
array
The line-of-sight position
Copy link
Member

Choose a reason for hiding this comment

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

Can we explicitly say this position is with respect to the system barycenter (if that is indeed the case)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Notes
-----
This is the radial velocity of the pulsar. For the radial velocity of the companion,
this must be multiplied by -1 times the mass of the pulsar divided by the mass of the companion.
Copy link
Member

Choose a reason for hiding this comment

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

Why not just add companion_radial_velocity() that takes the mass ratio as an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea. done

@paulray
Copy link
Member

paulray commented Sep 5, 2023

This is great! Thanks for adding it!

src/pint/models/timing_model.py Outdated Show resolved Hide resolved
src/pint/models/timing_model.py Show resolved Hide resolved
src/pint/models/timing_model.py Show resolved Hide resolved
@scottransom
Copy link
Member

Looks great! I'll 2nd Paul's thanks for doing this!

@scottransom scottransom merged commit 5b2c911 into nanograv:master Sep 14, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review This PR needs someone to review it so it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants