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

Disabled axes for pintk when not in timing model or data #1663

Merged
merged 13 commits into from
Nov 2, 2023

Conversation

dlakaplan
Copy link
Contributor

@dlakaplan dlakaplan commented Oct 23, 2023

  • Disabled "elongation" axes when no Astrometry component present
  • Disabled "orbital phase" axes when no Binary component present
  • Disabled "frequency" axes when <=1 frequency value present or when frequencies are not finite
  • Also changed default pintk fitters to downhill versions (Default fitter in pintk should be determined using Fitter.auto() #1665)
  • Also changed general is_binary property to rely on class inheritance rather than component name

Example on a barycentered dataset (so no Astrometry or Binary component and all event data):
Screen Shot 2023-10-23 at 12 43 27 PM

@dlakaplan dlakaplan linked an issue Oct 23, 2023 that may be closed by this pull request
@abhisrkckl
Copy link
Contributor

Disabled "frequency" axes when <=1 frequency value present

I think this should also be disabled when infinite-frequency TOAs are present.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (2977542) 68.50% compared to head (c8f98b9) 68.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1663      +/-   ##
==========================================
- Coverage   68.50%   68.46%   -0.05%     
==========================================
  Files         104      104              
  Lines       24276    24287      +11     
  Branches     4333     4336       +3     
==========================================
- Hits        16631    16628       -3     
- Misses       6560     6573      +13     
- Partials     1085     1086       +1     
Files Coverage Δ
src/pint/scripts/pintk.py 23.28% <100.00%> (ø)
src/pint/pintk/pulsar.py 33.43% <0.00%> (ø)
src/pint/pintk/plk.py 13.42% <9.09%> (-0.06%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dlakaplan
Copy link
Contributor Author

Done. also fixed a bug regarding initial log level

src/pint/pintk/plk.py Outdated Show resolved Hide resolved
src/pint/pintk/plk.py Outdated Show resolved Hide resolved
@dlakaplan dlakaplan linked an issue Oct 30, 2023 that may be closed by this pull request
@abhisrkckl
Copy link
Contributor

Can you also change the TimingModel.is_binary method to use isinstance instead of a string-based condition?

@dlakaplan
Copy link
Contributor Author

I did that. Not sure it's better (it requires a local import to avoid circular import errors).

@dlakaplan
Copy link
Contributor Author

I had to undo those changes because of various failures

@abhisrkckl
Copy link
Contributor

This looks good to me. Shall I merge this?

@dlakaplan
Copy link
Contributor Author

I think so

@abhisrkckl abhisrkckl merged commit 01a5ac0 into nanograv:master Nov 2, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants