-
Notifications
You must be signed in to change notification settings - Fork 101
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
get_observatory
shouldn't overwrite include_bipm
and include_gps
unless given explicitly
#1711
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1711 +/- ##
==========================================
+ Coverage 68.77% 68.78% +0.01%
==========================================
Files 105 105
Lines 24644 24643 -1
Branches 4407 4409 +2
==========================================
+ Hits 16948 16950 +2
+ Misses 6590 6588 -2
+ Partials 1106 1105 -1 ☔ View full report in Codecov by Sentry. |
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. BAT times should not get BIPM or GPS corrections applied, regardless of whether they are TZRMJD times or TOA times. BAT times are already fully corrected to TDB.
CHANGELOG-unreleased.md
Outdated
@@ -40,4 +40,5 @@ the released changes. | |||
- Consistent naming in `TimingModel.get_params_mapping()` | |||
- Better exceptions for unsupported/unimplemented binary models (BTX, MSS, etc.) | |||
- Emit warnings when `WaveX`/`DMWaveX` is used together with other representations of red/DM noise | |||
- `get_observatory()` no longer overwrites `include_gps` and `include_bipm` of `Observatory` objects unless explicitly stated. |
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.
I think we should add "Fixes issue #1691 (BIPM+GPS clock corrections incorrectly applied to BAT TOAs)"
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.
Added.
@paulray Can you please merge this? |
#1691 is a symptom of this, although the root cause is elsewhere.
#1707 was an attempt at fixing this.
Related issue - #1708