Merge EPTA version part 2#374
Conversation
vhaasteren
left a comment
vhaasteren
left a comment
There was a problem hiding this comment.
It needs to be linted and some docstrings need to be added. But overall this looks fine. Thanks!
| :param logf: use log frequency spacing | ||
| :param fmin: lower sampling frequency | ||
| :param fmax: upper sampling frequency | ||
| :param modes: option to provide explicit list or array of |
There was a problem hiding this comment.
The parameter idx needs a docstring
There was a problem hiding this comment.
added docstring for the parameter idx
|
|
||
| # compute the DM-variation vectors | ||
| Dm = (fref / freqs) ** idx * np.sqrt(12) * np.pi / 1400 / 1400 / 2.41e-4 | ||
|
|
There was a problem hiding this comment.
The matrix Dm is normalized differently from the one for createfourierdesignmatrix_red. This is not described in the docstring, and those factors are not explained anywhere.
Please make this consistent.
There was a problem hiding this comment.
It is the normalization difference between the enterprise and temponest default amplitude normalization for DM power law. There is the 12pi^2 from the definition of A, the scaling to 1MHz vs 1400 MHz and the DM constant. I added a bit of comments to this in the code.
There was a problem hiding this comment.
Understood, if it's a definition matter we should leave it.
Looking up the origin, this is a really pointless normalization though. This 12pi^2 comes from the definition of the characteristic strain h_c in the red noise spectral density. It has absolutely nothing to do with DM, and it was introduced by mindlessly copying code from the definition of red noise to DM. It makes me cringe. (Not a critique of this PR)
|
|
||
| @function | ||
| def createfourierdesignmatrix_general( | ||
| toas, |
There was a problem hiding this comment.
Question: is this not just a superset of the regular createfourierdesignmatrix? In other words, could it replace it?
There was a problem hiding this comment.
Yes, it is a superset of "createfourierdesignmatrix_red", "..._dm", "..._dm_tn" and "..._chromatic". Where the "chromatic" is a superset of "dm" by the way.
The "general" also allows to mask toas from chosen flags. It is pretty convenient as it is the only current way to make a selection on spatially correlated common signals (as far as I know).
There was a problem hiding this comment.
Actually, selecting on common signals is in another PR as well. Have a look:
There was a problem hiding this comment.
Missed that, very nice ! It still need to be merged if I've understood well.
There was a problem hiding this comment.
Correct. I wanted to review it the other day, but I wasn't sure whether it was relevant since it had been 2 years. I should ping @vallis again regarding it.
|
Review completed @siyuan-chen, I'm fine with it all. If all the linting is done we can merge to dev. |
3 participants
This pull request is part 2/2 to merge the EPTA enterprise into the NANOGrav version.
It adds more createfourierdesignmatrix functions, for EPTA work and also a general function for chromatic signals. And also associated changes in other functions to pass more arguments.