Move FD parameters to phase rather than dispersive delay - #15
Conversation
Fix for issue #14. Warning that this `breaks' old par files, but likely only by a small amount. This change aligns tempo2 and PINT. The idea is that FD parameters were interpreted as part of the dispersive delay, but are generally attributed to template matching issues, i.e. profile evolution, rather than propagation. This aligns FD and FDJUMP as well.
There was a problem hiding this comment.
🟡 Changes recommended
The new FD phase application currently narrows calculations to double (despite phaseJ being longdouble) and introduces minor comment/whitespace issues that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses issue #14 by changing how FD/FDDC/FDDI terms are applied so they no longer shift BAT/bbat (and therefore binary orbital phase evaluation), aligning Tempo2’s behavior with PINT by treating these as post-binary profile-evolution/template-matching phase effects.
Changes:
- Move FD and FDDC/FDDI contributions out of
tdis1(dispersive delay accumulator) and intophaseJinformResiduals.C. - Remove the FD/FDDC/FDDI block from
dm_delays.Csotdis1remains purely propagation/chromatic-delay related. - Bump the project version and update the project URL in
configure.ac.
File summaries
| File | Description |
|---|---|
tempo2.h |
Version macro bump to 2026.09.1. |
formResiduals.C |
Apply FD/FDDC as phase offsets alongside JUMP/FDJUMP instead of as a BAT-time correction. |
dm_delays.C |
Stop adding FD/FDDC to tdis1. |
configure.ac |
Version bump and update homepage URL to GitHub. |
Review details
Suppressed comments (1)
formResiduals.C:767
- Same precision issue for the FD polynomial: the current code forces the result through double (and uses double log/pow), despite phaseJ being longdouble. Using logl/powl keeps the phase term in longdouble all the way through.
for (k=0; k<psr[p].param[param_fd].aSize; k++) {
if (psr[p].param[param_fd].paramSet[k]==1 && psr[p].obsn[i].freqSSB>1)
phaseJ -= (double)(psr[p].param[param_fd].val[k] *
pow(log(psr[p].obsn[i].freqSSB/1e9),k+1)) * psr[p].param[param_f].val[0];
}
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: SixByNine <46102+SixByNine@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The behavioral reordering is significant and should be accompanied by a regression test (the repo has gtest end-to-end coverage) to ensure BAT/bbat is invariant under FD terms and the ordering bug cannot regress.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| /* Add FD parameters */ | ||
| // FD parameters used to be added to tdis1 in dm_delays, but more logical to add them here since | ||
| // we attribute these delays to profile evolution/template matching, and hence they affect phase | ||
| // rather than observation time. This affects the binary model. | ||
| if (psr[p].param[param_fddc].paramSet[0]==1 && psr[p].obsn[i].freqSSB>1) |
Fix for issue #14. Warning that this `breaks' old par files, but likely only by a small amount. This change aligns tempo2 and PINT.
The idea is that FD parameters were interpreted as part of the dispersive delay, but are generally attributed to template matching issues, i.e. profile evolution, rather than propagation.
This aligns FD and FDJUMP as well.