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

Modifications to the Pulsar class #365

Closed vhaasteren closed 3 months ago

vhaasteren commented 9 months ago

This PR does not add any new functionality or change any functionality. Instead, it slightly refactors the Pulsar class in a way that makes it easier to create derived classes without code replications.

Modifications:

These modifications allow, for instance, for the h5pulsar package to be used as a drop-in replacement instead of the base Enterprise pulsar classes. The h5pulsar derives it's pulsar classes from Enterprise without code duplication. The modifications in this PR are completely independent from that package though, and simply make the code more modular.

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.99%. Comparing base (daede07) to head (61ba269).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/nanograv/enterprise/pull/365/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/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nanograv) ```diff @@ Coverage Diff @@ ## dev #365 +/- ## ========================================== + Coverage 86.74% 86.99% +0.24% ========================================== Files 13 13 Lines 3115 3144 +29 ========================================== + Hits 2702 2735 +33 + Misses 413 409 -4 ``` | [Files](https://app.codecov.io/gh/nanograv/enterprise/pull/365?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/365?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=) | `93.54% <100.00%> (+1.44%)` | :arrow_up: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/nanograv/enterprise/pull/365?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/365?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...61ba269](https://app.codecov.io/gh/nanograv/enterprise/pull/365?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).
AaronDJohnson commented 4 months ago

@vhaasteren does storing the string data change the memory requirement for the object significantly?

vhaasteren commented 4 months ago

@vhaasteren does storing the string data change the memory requirement for the object significantly?

Significantly is a relative term. The tim files for some pulsars are several MB in size. Must be around 20MB for the largest pulsars in the IPTA? I didn't even think about it, but I suppose for an entire PTA it could add up.

Currently, it's stored if we set drop_t2pulsar=False or drop_pintpsr=False. Are you perhaps suggesting to make that a different option? My thinking was that, since saving the pulsar is optional, the usual analysis wouldn't be hindered in any way. The default is to drop the pulsar, meaning the par and tim files aren't saved either