Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modifications to the Pulsar class #365

Merged
merged 10 commits into from
May 9, 2024
Merged

Conversation

vhaasteren
Copy link
Member

@vhaasteren vhaasteren commented Nov 9, 2023

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:

  • the classes PintPulsar and Tempo2Pulsar classes now have attributes drop_not_picklable, which initially was implemented in the base class.
  • The par and tim files are also saved in strings as part of the pulsar object.
  • Designmatrix units are saved
  • The requested timing package in Pulsar is attempted to be used inside of a try block
  • Add extra unit tests for full coverage

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.

@vhaasteren vhaasteren self-assigned this Nov 9, 2023
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

Impacted file tree graph

@@            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 Coverage Δ
enterprise/pulsar.py 93.54% <100.00%> (+1.44%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daede07...61ba269. Read the comment docs.

@AaronDJohnson
Copy link
Collaborator

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

@vhaasteren
Copy link
Member Author

vhaasteren commented Apr 23, 2024

@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

@vhaasteren vhaasteren mentioned this pull request May 7, 2024
@vhaasteren vhaasteren removed the request for review from paulthebaker May 8, 2024 05:33
@vhaasteren vhaasteren merged commit 37e24d2 into nanograv:dev May 9, 2024
13 checks passed
@vhaasteren vhaasteren mentioned this pull request May 9, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants