search

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

Properly address units in PintPulsar position #370

Closed vhaasteren closed 3 months ago

vhaasteren commented 9 months ago

This fixes #363

By using astropy.units to convert to units.rad, it doesn't matter what units PINT returns, the position is read in radians.

codecov[bot] commented 9 months ago

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 88.37%. Comparing base (5c1e455) to head (c58f840). Report is 22 commits behind head on dev.

:exclamation: Current head c58f840 differs from pull request most recent head 7b7eb22. Consider uploading reports for the commit 7b7eb22 to get more accurate results

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/nanograv/enterprise/pull/370/graphs/tree.svg?width=650&height=150&src=pr&token=7Sjk8cLA85&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv)](https://app.codecov.io/gh/nanograv/enterprise/pull/370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) ```diff @@ Coverage Diff @@ ## dev #370 +/- ## ========================================== - Coverage 88.44% 88.37% -0.07% ========================================== Files 13 13 Lines 3037 3036 -1 ========================================== - Hits 2686 2683 -3 - Misses 351 353 +2 ``` | [Files](https://app.codecov.io/gh/nanograv/enterprise/pull/370?dropdown=coverage&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://app.codecov.io/gh/nanograv/enterprise/pull/370?src=pr&el=tree&filepath=enterprise%2Fpulsar.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv#diff-ZW50ZXJwcmlzZS9wdWxzYXIucHk=) | `91.62% <50.00%> (-0.46%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/nanograv/enterprise/pull/370/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/nanograv/enterprise/pull/370?dropdown=coverage&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://app.codecov.io/gh/nanograv/enterprise/pull/370?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv). Last update [daede07...7b7eb22](https://app.codecov.io/gh/nanograv/enterprise/pull/370?dropdown=coverage&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).
vhaasteren commented 9 months ago

Note that the lines not covered by tests were already not covered.

I think it's actually worthwhile to include a NG 5yr dataset (say, B1855+09) in the test data (under tests/data/), so we can load a quick PINT pulsar with RAJ/DECJ parameters and also run those tests. PINT also includes that pulsar in its test data. Having a pulsar with RAJ/DECJ that is not 1713 is useful to have for other unit tests that still need to be implemented as wel. Right now we only have 1713.Sep.T2.par/tim

vhaasteren commented 9 months ago

Also note that the designmatrix and model parameter units are not addressed by this PR. That is a separate issue that should be addressed in a more comprehensive PR

vhaasteren commented 4 months ago

This one depends on #382 for the added test to pass. So @tcromartie, this last test I added was missing (uncovered lines). If it had been there from the start, your PR #382 would have never been happening.

Other than that, this PR is ready