nanograv / enterprise

ENTERPRISE (Enhanced Numerical Toolbox Enabling a Robust PulsaR Inference SuitE) is a pulsar timing analysis code, aimed at noise analysis, gravitational-wave searches, and timing model analysis.
https://enterprise.readthedocs.io
MIT License
64 stars 65 forks source link

Create combined EFAC + EQUAD noise object with tempo/tempo2 convention #315

Closed vallis closed 2 years ago

vallis commented 2 years ago

Instead of defining separate MeasurementNoise and EquadNoise signals, one would use CombinedWhiteNoise(efac=..., log10_equad=..., selection=...).

vallis commented 2 years ago

To complete the transition and avoid confusion, EquadNoise(log10_equad=...) would be renamed TNEquadNoise(log10_tnequad=...). This will break all code that calls EquadNoise. Are we ready for that?

codecov[bot] commented 2 years ago

Codecov Report

Merging #315 (8aad5a0) into master (813f9e3) will decrease coverage by 0.00%. The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
- Coverage   88.01%   88.00%   -0.01%     
==========================================
  Files          13       13              
  Lines        2970     2977       +7     
==========================================
+ Hits         2614     2620       +6     
- Misses        356      357       +1     
Impacted Files Coverage Δ
enterprise/signals/signal_base.py 90.21% <50.00%> (-0.11%) :arrow_down:
enterprise/signals/white_signals.py 98.47% <100.00%> (+0.06%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 813f9e3...8aad5a0. Read the comment docs.

scottransom commented 2 years ago

My opinion is to break things. It seems that most people have been unknowingly using it incorrectly anyways! Which is worse?

Hazboun6 commented 2 years ago

@vallis Would anything break if we changed the names of the Tempo-style parameter EQUAD to something else? Echoing something from a DWG telecon, maybe breaking the ability for a user to use old EQUAD values is the safest thing to do here. For instance changing the new EQUAD name so that the name comes out as J1909-3744_log10_tequad where the t stands for either Tempo or Timing (since all of the timing packages use that convention). Or another name more varied from T2EQUAD.

@kdolum do you think this would fix the issues you were foreseeing?

kdolum commented 2 years ago

@Hazboun6, I'm not up on the current and proposed new set of parameter names, but I think what you're suggesting would address my issue. My goal is that if someone has an old parameter file or notebook or set of instructions that refers to the old name, and they try to use it with new code, they get an error instead of a wrong answer. Then they can hunt down the documentation of the change and update their files or process them with an old version or whatever.

vallis commented 2 years ago

Yes, I think this is the right idea. I was already going to break things one way (using the “old style” equad objects with the wrong parameters), but we should do it both ways (also using “new style” objects with old chains).

This means the parameter name will be different from tempo/tempo2/pint, so an explicit mapping will be required. But that’s ok. I’m ambivalent about the name: I have tnequad for the temponest style; the tempo/tempo2/pint style could be t2equad (but pays undue homage to tempo2), tequad (but it’s hard to pronounce), trequad (for traditional), thequad (if you like puns)…

Opinions?

Hazboun6 commented 2 years ago

We could take a quick Slack poll in the timing channel?

vallis commented 2 years ago

OK, so we're going with T2EQUAD after hearing the timers. Note that I've changed the interface again. EFAC and EQUAD (tempo-style) are now consolidated under MeasurementNoise(efac=..., log10_t2equad=..., selection=...); the signal_id is "measurement_noise". To use EFAC only, leave out log10_t2equad.

Hazboun6 commented 2 years ago

@vallis Are we good to merge this?

vallis commented 2 years ago

I think so... I haven't looked at the e_e side, but as long as you have removed EquadNoise and renamed the parameter in the MCMC setup, we should be good.

I will proceed with the merge and release. So far I have been tagging and releasing from the branch, which however leaves master with an old version number. So I will tag and release from master, then copy into a new branch for bug fixes.