nanograv / pint_pal

A long-lived repository for NANOGrav Pulsar Timing workflows and analysis.
MIT License
6 stars 19 forks source link

Added convert_tcb2tdb=False to all parameters #68

Closed mtlam closed 3 months ago

mtlam commented 3 months ago

To allow us to import in the notebooks using the latest versions of PINT, this adds convert_tcb2tdb=False to all parameters.

As an aside, I have also replaced all use of "LABMDA" with "LAMBDA"

rossjjennings commented 3 months ago

We had an extensive discussion about this, and whether this relatively simple patch is the best way to fix it, on today's git-flowers call. The underlying issue here is that the add_parameter() function in PINT that PINT Pal uses for its F-tests is intended to be used by developers of new models and model components, and not in the way it is being used here, to add parameters that PINT already knows about.

The way it is being used here, users have to specify many things that PINT should already know, such as the type of the parameter, its units, and so forth. This includes the convert_tcb2tdb and tcb2tdb_scale_factor parameters at issue here. However, several PINT users have independently arrived at the conclusion that this is the correct way to do things based on the PINT documentation.

In my opinion, the best solution would be to add a new function to PINT that would facilitate adding parameters in the sense that end users want, allowing them to avoid the internal machinery designed for model development. @scottransom disagreed with this, suggesting that, rather than attempting to manipulate model objects, we should instead be using par files represented as StringIO objects. By doing so, we would avoid adding complexity to PINT's already "baroque" model builder code, which in the longer term should be rewritten. But @mlam also pointed out that moving away from the need to manipulate par files was always one of PINT's development goals.

In any case, as a more complete solution to the problem of adding parameters is likely to take some time, and this is keeping the notebooks from being usable right now, I am going to go ahead and merge this PR as a temporary fix. (Nothing is more permanent, I know, but I do intended to work on a PINT PR with my proposed fix as well).

rossjjennings commented 3 months ago

I know I said I would merge this above, but I just noticed one thing: @mtlam, should this PR target the NG20 branch instead of main?

mtlam commented 3 months ago

Crud, you're right, I just changed that to NG20.

rossjjennings commented 3 months ago

OK, great! Merging it now...