Guess binary model & add conversion script #1695
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1695 +/- ##
==========================================
- Coverage 69.11% 68.97% -0.15%
==========================================
Files 105 106 +1
Lines 24749 24807 +58
Branches 4416 4424 +8
==========================================
+ Hits 17106 17110 +4
- Misses 6534 6586 +52
- Partials 1109 1111 +2 ☔ View full report in Codecov by Sentry. |
|
Very nice. Would it make sense to further build a function to make this change (sort of like the TCB->TDB function) based on the best-guess? A scripts too? |
|
I think such a script would be convenient to have, and it's in line with not supporting TCB but still providing a script. Same philosophy. Sure, I can add that. As far as I understood from the timers, the only transformation I would need to do are for the DDK parameters KOM/KIN, which have a different definition in T2 compared to DDK. That's your expertise, @dlakaplan, is that the only transformation that is necessary? Also, although what I wrote works, I now feel that it is not elegant enough, and it adds complexity to the logic. My thought is be to re-factor Something that I need feedback on is the |
I think so. There are routines in |
Right, I see. But... I also see that upon reading, the KOM/KIN parameters are converted in the BinaryDDK to the AU convention, and SINI is removed. I am now a bit puzzled by that logic. SINI is only there if we are reading in a Tempo2 parfile, which would have KOM/KIN in the IAU convention to begin with. But BinaryDDK needs them in the DT96 convention. Isn't that a bit of a weird state for the parfile to be in? My current conversion function will remove SINI and convert KOM/KIN. Anyway, a somewhat important question: it looks like the model_builder actually allows TCB if you set the option for it. By default PINT throws an error for tcb, but you can force it to read a model with TCB. Reason this is important is that the neatest design for the script would be for the model_builder to read/convert/write the parfile. But in order to convert with the model builder, it needs to be able to read/convert them with options. This is (close to) supporting T2 models. Perhaps this is a good topic for a telecon |
|
@dlakaplan, I've just put together an initial t2binary2pint script, and I'm realizing that such a parfile will always be TCB, so it's kind of superseding functionality of tcb2tdb. In fact, I've just implemented it in the ModelBuilder class in the same way. A single tool that converts tempo2 -> PINT would be more general though, and perhaps not something we'd want to support. Shall I just leave it as a separate? |
vhaasteren
changed the title
vhaasteren
changed the title
vhaasteren
added
enhancement
ready for new iteration review
|
Added iteration review tag, because the converted binary model is likely not correct. For Specific question Use the tool |
What do you think is wrong? |
Oh... I don't know why I thought it was wrong actually. The residuals look the same as they are with libstempo/tempo2. And the original parfile has: The new one has: I looked at the KOMIAU/KINIAU parameters, and somehow decided they were different. Ok, fine, I'll add some tests for the conversion script and mark this PR as completed. It seems fully functional now. |
vhaasteren
added
awaiting review
|
Should be ready now |
|
Can you just paste a few examples in here? |
|
PPTA DR3 J1713 (skipping JUMP parameters): becomes: And PPTA DR3 J0613: becomes Edit: I see now that the PPTA release doesn't even declare 'UNITS' in the par file. However, I suppose that should be irrelevant for checking the functionality on display here. All above was run with |
|
Please add an entry in |
|
Also, please add the |
…toas and updated Changelog unreleased
|
Done |
|
Can you please merge nanograv:master? It fixes the macos-latest issue. I think this is pretty much ready to go otherwise. I'll merge this once all the tests pass. |
Done. I also added one more unit test that checks for an Exception when the binary model cannot be determined |
When a binary model is used that PINT does not know about (e.g. T2), an error is thrown. Sometimes the underlying binary model that the user wants to use is easy to guess. We can provide that guess to help the user
This PR provides a utility function that can guess the binary model from a parfile dictionary, and the UnknownBinaryModel error is modified to include a hint for the user as to which binary model they may want to try, such as this one:
UnknownBinaryModel: Pulsar system/Binary model component T2 is not provided. Perhaps use DDK?An error is still thrown, and no attempt is made to convert parameter values. A conversion script is also available, called
t2binary2pint. This script just uses theModelBuilderclass with the optionallow_T2=True. That will decide on the best binary model, and in the case of DDK it will convert the KOM/KIN parametersTODO