Enterprise falls back to PINT if no TEMPO2

[aarchiba]

This has three significant changes:

• If you run Pulsar(par, tim) without specifying PINT vs TEMPO2, one of two things will happen:
  • If you have TEMPO2 installed, you get a Tempo2 pulsar object as before.
  • If you don't have TEMPO2 installed, instead of an exception you now get a PintPulsar object.
• Almost all tests now run on PintPulsar objects if TEMPO2 is not available; the test suite passes and testing is reasonably complete.
  • Tests that require TEMPO2 (for example the par file uses BINARY T2) are skipped.
  • Tests that fail with PINT (inflate/deflate) are marked as expected to fail (xfail).
  • A few tests try to use both PINT and TEMPO2, skipping the TEMPO2 version if it's not available. (These tests were already run with both.)
• If PINT is not available, Enterprise continues to run using only TEMPO2 pulsar objects; the test suite likewise runs on almost all tests.

In short: you can use Enterprise transparently without TEMPO2. Also: a CI run now checks that Enterprise runs fine using PintPulsar for everything, and another checks that Enterprise runs fine without PINT. (Getting Enterprise to install when one or other dependency is missing is awkward but doable.)

[aarchiba]

This is particularly valuable in light of #339 ; no seg faults if no TEMPO2.

[aarchiba]

Still to do:

• [x] Update CI so a run is done without TEMPO2 installed.

[codecov[bot]]

Codecov Report

Merging #340 (bcd3892) into master (5ef5ff4) will decrease coverage by 0.05%. The diff coverage is 80.00%.

Additional details and impacted files

[](https://codecov.io/gh/nanograv/enterprise/pull/340?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) ```diff @@ Coverage Diff @@ ## master #340 +/- ## ========================================== - Coverage 88.37% 88.33% -0.05% ========================================== Files 13 13 Lines 3012 3026 +14 ========================================== + Hits 2662 2673 +11 - Misses 350 353 +3 ``` | [Impacted Files](https://codecov.io/gh/nanograv/enterprise/pull/340?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) | Coverage Δ | | |---|---|---| | [enterprise/pulsar.py](https://codecov.io/gh/nanograv/enterprise/pull/340?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv#diff-ZW50ZXJwcmlzZS9wdWxzYXIucHk=) | `91.60% <80.00%> (-0.46%)` | :arrow_down: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/nanograv/enterprise/pull/340?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/nanograv/enterprise/pull/340?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv). Last update [5ef5ff4...bcd3892](https://codecov.io/gh/nanograv/enterprise/pull/340?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv).

[aarchiba]

Question: how do we make libstempo an optional dependency? The no-tempo run fails at pip install -r requirements_dev.txt because tempo2 isn't available (it didn't on my machine, for some reason). I believe I can fake this for now.

[aarchiba]

The no-tempo2 installation is a bit of a hack, as we can't really tell pip "don't bother with this required dependency" and so it insists on installing libstempo, which fails if tempo2 is not available.

Long-term, we could make the tempo2 dependency optional - pip install enterprise[tempo2] - but we can't really make that the default (except on conda?).

Short-term, we might be able to trick libstempo into claiming to install even though tempo2 isn't available. This could perhaps be done most easily with a pip-install-fake-libstempo.py that creates a fake pure-python libstempo of the correct version and does pip install -e on that.

[aarchiba]

To get adequate coverage of the patch I added a few tests; several of them are XFAIL because the behaviour of Pulsar() is bad but I hesitate to fix it here. #332 also calls for improvements to Pulsar().

[aarchiba]

We have three conditions for coverage:

1. The overall coverage of the entire set of CI runs must meet some threshold implemented by codecov.
2. The entire set of CI runs must cover some fraction of the lines in the PR, as implemented by codecov.
3. Every individual CI run must cover at least 85% (now 75%) of the code base; this includes CI runs where PINT or TEMPO2 are not available.

I think the third requirement is awkward and inconvenient from a CI point of view, although it is one that can be checked locally with make test (unlike the other two). I'm not sure what the right solution to this is.

Perhaps we could have two makefile targets: make test runs with a local coverage requirement, while make test-ci still gathers coverage information but does not fail if this single CI run has insufficient coverage. The existing codecov setup will make the PR fail CI if the overall coverage is insufficient.

[aarchiba]

The changes to the code base are quite minor; just pulsar.py, mostly improved error handling. The bulk of the changes here are to the test suite, mostly just annotating tests with which program they need, but there are a few tests I had to convert to pytest style.

[aarchiba]

I'm not sure what's up with the HTTP 404s - this occurs when astropy is trying to update its leap seconds file. It doesn't happen on my local machine, and it seems to only happen with the old python 3.7, pint 0.8.8, astropy 4.3.1 version; with astropy 5.2.1 the problem disappears. There were substantial upgrades to astropy's remote data fetching with astropy 5, so this may help explain why the problem only occurs with very old versions.

[aarchiba]

Something has happened to prevent the coverage upload from the "no tempo2" version. I'm not sure what; maybe a passing network problem.

[vhaasteren]

I do not like the hoops we are jumping through here to have libstempo installed by default, but to allow for a no tempo2 version. Our situation is perhaps somewhat unique that we have two independent packages, neither of which is ideal:

• Tempo2/libstempo are still a pain to install on some systems, despite progress in this regard
• PINT is properly packaged, but slow, and newer so less well-supported

Unfortunately, Python's packaging system doesn't directly support conditional dependencies based on the presence of other packages at install time. The closest built-in feature is the extra requires functionality, with those square brackets.

My suggestion to deal with this in a consistent way would be to

• Not have a required dependency on either libstempo or PINT
• Provide good documentation on the need for one of the two (in case we want to use par and tim files)
• Perform a runtime check for either of the two packages (and an associated warning)
• Allow for Enterprise use without a timing package (e.g. PR #361)

Of course the problem with this approach is that some build scripts and container scripts currently rely on libstempo being installed automagically, so if we make this change there will be things that break.