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

WIP: basic support for polyco doppler and rms #1713

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dlakaplan
Copy link
Contributor

Previously the Doppler and RMS fields for polycos were set to 0 explicitly (#595, maybe #1369).

I have added computation of the Doppler shift and RMS. However, I am unsure how to test them since we don't have comparisons against external polycos (and as per #1369 it is not clear if they would succeed).

The Doppler shifts here seem very similar to those in #595. The RMS values are ~1 dex lower - this may be due to differences in how the polycos are computed, or an error in units.

For reference, when I use the same routine that #595 did I now get:

In [7]: polycos[['doppler','logrms']]
Out[7]: 
<Table length=21>
     doppler              logrms      
    float64[1]           float64      
------------------ -------------------
0.8277697583935151  -10.05876484179034
0.8214061155480782 -10.187670625757141
0.8252718627984125  -10.06929480188947
0.8333408761473038  -10.06110378949105
0.8333122527054521 -10.262116883531544
               ...                 ...
0.8192315787385138 -10.041679764534035
0.8191044627908792  -9.993124663597873
0.8270383391991994 -10.054704235766907
0.8307247245349052  -10.12410055651501
0.8241657455674353 -10.016446104786814
0.8166066521214685 -10.011765829681192

@dlakaplan dlakaplan added the bug fix Bug fixing pull request label Jan 15, 2024
@dlakaplan dlakaplan linked an issue Jan 15, 2024 that may be closed by this pull request
@dlakaplan dlakaplan marked this pull request as draft January 15, 2024 21:02
@dlakaplan
Copy link
Contributor Author

@mcknightryan, @emmacarli: let me know what you think.

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b8d8f7c) 68.76% compared to head (39f31de) 68.97%.
Report is 76 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1713      +/-   ##
==========================================
+ Coverage   68.76%   68.97%   +0.20%     
==========================================
  Files         105      105              
  Lines       24636    24704      +68     
  Branches     4404     4409       +5     
==========================================
+ Hits        16942    17039      +97     
+ Misses       6589     6560      -29     
  Partials     1105     1105              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dlakaplan dlakaplan added the awaiting review This PR needs someone to review it so it can be merged label Feb 6, 2024
@dlakaplan
Copy link
Contributor Author

The main work here is done, but there are no specific tests. That's why it's still a draft. If anybody can think of proper tests let me know.

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 bug fix Bug fixing pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polycos with Doppler shifts 0
1 participant