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

Setup of DMX uncertainties incompatible with NG 15 yr timing pipeline #327

Closed NihanPol closed 1 year ago

NihanPol commented 1 year ago

Hey folks, there was a change in how enterprise reads in the DMX values and uncertainties, where the values are being cast to floats (see attached image).

While this is fine for DMX values, there can be instances in the timing pipeline where the DMX uncertainties are not defined until a noise model is produced for any pulsar. But since the current version of enterprise cannot cast these missing DMX uncertainties (NoneType) to float, it throws an error when setting up PintPulsar and prevents the noise run from being executed.

Since enterprise does not use the DMX uncertainties in the timing model, can we revert back to when NoneType values are acceptable for DMX uncertainties?

image

vallis commented 1 year ago

Sorry for the holiday delay, Nihan: these changes were made so that no float80/float128 values would be left in enterprise Pulsar, which needs to be readable on platforms that don't have those types (e.g. Apple M1/M2).

It would be OK to change this to, say,

None if model[par].uncertainty_value is None else float(model[par].uncertainty_value)

However, let me ask: is None indeed the idiom in PINT for parameters that do not have an uncertainty? Should it be a float nan instead? If that's the case this issue should be fixed on the PINT side.

Libstempo just passes on whatever tempo2 has (that may be 0 in fact, but the parameters wouldn't have the fit flag set).

If that's not the case, we can push the above fix to master quickly.

JPGlaser commented 1 year ago

@vallis @NihanPol , can we get this pushed to master and a version tagged asap? This is going to cause a slowdown with the final WB run.

~ Joe G.

swiggumj commented 1 year ago

One section of the TWG notebook checks whether new DMX windows are necessary (this can happen if the average DM wanders too far from the frozen DM value, or if any new TOA excision has happened) and if so, replaces them with fresh windows. For various reasons, the prefit model is used for the noise run, so if new windows have been added and a fit has not occurred, uncertainties will not yet exist.

vallis commented 1 year ago

OK, I will push this today. Should I bump the pint-pulsar requirement also? We're at 0.8.3 currently, I see 0.9.3 was released recently.

swiggumj commented 1 year ago

You could, but I don't think it actually makes a difference here. One thing I'll note is that since the timing notebooks/code are now public (pint_pal), we can in principle design a test for the enterprise test suite so that we can catch these sorts of things ahead of time. We're currently discussing doing this with the PINT team.

vallis commented 1 year ago

Merged in #329.