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

More flexible read/write for polycos #1371

Merged
merged 25 commits into from Aug 24, 2022

Conversation

dlakaplan
Copy link
Contributor

@dlakaplan dlakaplan commented Aug 4, 2022

Changes:

  • Can now read/write from/to a Path object or a stream (like io.StringIO).
  • Registering formats done as a class method. Done once on import for default formats (just tempo), can be done later if needed.
  • Read polycos and generate from timing model now done as class methods that return new objects
  • Documentation cleanup/improvements.
  • implemented len(), __getitem__ for polycos
  • implemented __getitem__ for Phase objects

@dlakaplan
Copy link
Contributor Author

P.S. Polyco calculation is slow, because it relies on making a lot of TOAs and that is slow. Which then makes testing slow (~5min just for test_polcos.py. So if we make progress with any other speedups it will help a lot here.

@dlakaplan
Copy link
Contributor Author

Note that the change to the Phase object to allow indexing changes some assumed behavior. Previously, with a Phase object p, p[0] would be the integer part and p[1] would be the fractional. Now p[0] is the first item (integer & fractional) and you can still use p.int and p.frac to access the other attributes. If this is an issue I can revert it but I found it useful for some testing.

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #1371 (8a3eb2d) into master (116b0ea) will increase coverage by 0.03%.
The diff coverage is 68.81%.

@@            Coverage Diff             @@
##           master    #1371      +/-   ##
==========================================
+ Coverage   62.22%   62.26%   +0.03%     
==========================================
  Files          89       89              
  Lines       20232    20260      +28     
  Branches     3649     3652       +3     
==========================================
+ Hits        12589    12614      +25     
+ Misses       6859     6856       -3     
- Partials      784      790       +6     
Impacted Files Coverage Δ
src/pint/phase.py 93.87% <66.66%> (-3.80%) ⬇️
src/pint/polycos.py 78.11% <68.23%> (+1.24%) ⬆️
src/pint/mcmc_fitter.py 40.11% <100.00%> (ø)
src/pint/scripts/event_optimize.py 63.31% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aarchiba
Copy link
Contributor

P.S. Polyco calculation is slow, because it relies on making a lot of TOAs and that is slow. Which then makes testing slow (~5min just for test_polcos.py. So if we make progress with any other speedups it will help a lot here.

Some of the slow parts of working with TOAs are less with barycentric TOAs, so perhaps those could be the default for polyco tests?

More broadly, I don't think there's a lot we can do to speed up polyco generation, since those slow TOA calculations are exactly what is needed to get things as seen at specific observatories. (Incidentally, how are clock corrections handled?)

If we were willing to diverge from the TEMPO approach, we might find we got equally good or better approximations for the same or fewer TOAs if we distributed the TOAs according to the zeros of Chebyshev polynomials. Doing polynomial fitting on these points amounts to working in the Chebyshev basis, which is the right basis for limiting maximum absolute deviation.

We might be able to get a minor speedup by generating all the TOAs at once and reusing any that are needed for more than one polyco entry (if we do that). There's also an issue pointing out that for many satellite applications, polycos are only needed for specified Good Time Intervals, and we could skip generating any outside those.

Major speedups are going to require a careful analysis of where PINT spends its time when you're working with TOAs - I don't think that there is necessarily duplicated effort, but there may be many places where unnecessary precision is costing us a lot of time.

@aarchiba
Copy link
Contributor

Note that the change to the Phase object to allow indexing changes some assumed behavior. Previously, with a Phase object p, p[0] would be the integer part and p[1] would be the fractional. Now p[0] is the first item (integer & fractional) and you can still use p.int and p.frac to access the other attributes. If this is an issue I can revert it but I found it useful for some testing.

Useful for testing frequently means more flexible for users.

@dlakaplan dlakaplan changed the title WIP: More flexible read/write for polycos More flexible read/write for polycos Aug 11, 2022
@dlakaplan
Copy link
Contributor Author

dlakaplan commented Aug 11, 2022

Examples of new Phase interface:

from pint.phase import Phase
import numpy as np
p = Phase(np.arange(10),np.random.random(10))
print(p.int)
print(p.frac[:5])
print(p[::2])
i,f = p

all work

@dlakaplan
Copy link
Contributor Author

I removed the __getitem__ method for the Phase objects so that we can potentially merge this without waiting for #1375 to converge. I think the remaining changes (initialize polycos better, read/write from streams) are uncontroversial.

@dlakaplan dlakaplan added the awaiting review This PR needs someone to review it so it can be merged label Aug 18, 2022
@aarchiba aarchiba merged commit 31c3865 into nanograv:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review This PR needs someone to review it so it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants