nanograv / PINT

PINT is not TEMPO3 -- Software for high-precision pulsar timing
Other
121 stars 101 forks source link

Dependency on fftfit #231

Open paulray opened 7 years ago

paulray commented 7 years ago

Currently the PINT script event_optimize requires the fftfit module, which is part of PRESTO. This adds a (fairly complex) dependency and makes it difficult to test in travis-ci. I wrote a test for event_optimize, but it is currently disabled in setup.cfg (though it can be run manually since it works fine for people with PRESTO installed).

@scottransom we should consider how to remove this dependency

matteobachetti commented 7 years ago

@paulray @scottransom I made a full-python implementation for Stingray. It's a little rough, but you can use it as a backup if Presto is not installed. Code here towards the end: https://github.com/StingraySoftware/stingray/blob/master/stingray/pulse/pulsar.py

scottransom commented 7 years ago

So I think we want to consider if files in the "examples" directory should really be part of the official tests and dependency lists. I was surely thinking they would not be. I had thought of them as examples of how you might use PITN in non-TEMPO-like ways. And event_optimize is a great example of that. If that is the case, then it should be able to have other dependencies beyond those required for PINT (like psr_uttils and emcee and corner).

If others don't like this, then a pure-python version as @matteobachetti mentioned is completely fine with me. The only reason why PRESTO uses Joe Taylor's original Fortran code is that several collaborators wanted it for consistency.

paulray commented 7 years ago

Currently, I think event_optimize is the most popular use for PINT. I'm not really aware of people doing any production work with PINT except for the people who use event_optimize. So, I promoted it from examples to scripts, and added a unit test. Currently that test is disabled because of the PRESTO dependency.

I did add the emcee and corner dependencies since they are both easily pip-installable and are likely to be used in the future as soon as PINT gets an MCMC fitter for normal pulsar timing and not just event_optimize.

paulray commented 7 years ago

One other thought. I agree with @scottransom that the official test suite probably doesn't need to run the examples. However, every so often (maybe as a pre-requisite for tagging a version) someone should run through the examples and the iPython notebooks to make sure that API changes have not broken them.

aarchiba commented 5 years ago

The python version is only a handful of lines. Is there any reason not to pull it into PINT?

aarchiba commented 5 years ago

I think event optimization is a valuable enough use for PINT that it should be moved out of the examples/scripts and into the main code base, and thus tested. Best if it doesn't need any extra dependencies, but is depending on Stingray a problem? How difficult is it to install - if it's just available from PyPI and conda why not?

matteobachetti commented 5 years ago

Hi @aarchiba , Stingray already depends (optionally) on PINT. I would suggest, in order to avoid circular imports, to just copy the fftfit function into PINT. It could be a good opportunity to test it properly and improve it, actually! Stingray is MIT-licensed, there is no problem doing that. If people agree, I can open a pull request

paulray commented 5 years ago

I think that would be great!

aarchiba commented 5 years ago

I think a good solution might be for PINT to export an fftfit that is a drop-in replacement for presto's fftfit. From stingray, or I have code. We can write a set of test cases that do a one-to-one comparison with presto (but are skipped if presto isn't installed) to validate the PINT version (above and beyond intrinsic validation tests). Stingray could then import PINT's fftfit and get a nice fast one from presto if that's available.

matteobachetti commented 5 years ago

@aarchiba, totally agree. We need to create an fftfit module with methods mimicking PRESTO's (e.g. cprof()). The output of fftfit.fftfit() needs to be changed wrt Stingray's version. I started working on it then had to cope with other stuff. If you have something already in the right format, feel free to use that.

paulray commented 4 years ago

I'd like to get this dealt with. Matteo, where is the fftfit code in HENDRICS?

matteobachetti commented 4 years ago

https://github.com/StingraySoftware/stingray/blob/d64644b3ed69e5cce0c94d73da4bc446aa584b4e/stingray/pulse/pulsar.py

matteobachetti commented 4 years ago

Hi @paulray: I implemented a better version of fftfit, going back to the original paper and borrowing a few tricks from PRESTO (an endless source of awesomeness @scottransom ). You can find here: https://github.com/NuSTAR/nustar-clock-utils/blob/master/nuclockutils/diagnostics/fftfit.py I will soon also implement it in Stingray.

paulray commented 4 years ago

Great! Since @scottransom and Thankful are working on event_optimize, maybe they can incorporate this into PINT so we can remove that dependency!

aarchiba commented 4 years ago

I have Opinions on the right way to do this, but more usefully I have a test suite that evaluates how well my own fftfit-like implementation works. I'll try incorporating some of this code into PINT, ideally with several implementations we can compare.

paulray commented 4 years ago

Wonderful, Anne! That would be great!

scottransom commented 4 years ago

I agree! I definitely trust your Opionions, @aarchiba! Thanks!

aarchiba commented 4 years ago

I have a version that I set up to try to construct RXTE TOAs for Kes 75 (don't ask). It's pretty minimal in terms of functionality but I made the mistake of trying to use hypothesis to test it and goodness is it hard to make a really bulletproof version that guarantees a reasonable precision. So now I want to try my test suite on other fftfit implementations.

But one Opinion in particular is maybe relevant to event_optimize: if you're working with photons, especially if they as sparse as Fermi photons, you are probably better going through empirical Fourier coefficients instead of binning them into a profile and then taking the FFT. I have some tools for working with profiles in this representation, including a different (sigh) fftfit implementation.

Getting really reliable uncertainties, even for synthetic data, is the real kicker, of course.

matteobachetti commented 4 years ago

@aarchiba that would be great! I'd be super happy to see the actual performance against other implementations (and scrap it and use other implementations if they work better and their license allows it ;) ).

kerrm commented 3 years ago

To seed some entropy: at least for Fermi, there is an alternate path, namely using all of the machinery in templates. This provides implementations of various primitives (gaussians, lorentzians) and it widely used in the Fermi TOA code. I am using it in my new timing pipeline. It offers a few key features:

I do all of my fitting with pickled instances of these templates. It's a little painful when versions change, but one alternative is the text output we used to use, which allows one to make the object instance from ASCII text tabulating the properties of the primitives.

astrogewgaw commented 2 years ago
Hey everyone 😁 ! Just wanted to let y'all know that, after talking to @scottransom (ref: **https://github.com/scottransom/presto/issues/176**), I have been working on separating the Fortran code for the FFTFIT algorithm into its own Python package here: **https://github.com/astrogewgaw/fftfit**. I have essentially done the same thing as `PRESTO` does to bind it to Python, namely via `f2py` and `numpy.disutils`. I hope that this package can help with comparing the pure Python implementations, written by @aarchiba, @matteobachetti, and others, to the original implementation. It can also help with the performance comparisons, as @matteobachetti has suggested, probably serving as a baseline. I hope that this would provide an implementation of the FFTFIT algorithm that is: 1. Usable from Python. 2. Accessible, via the MIT License. 3. Available to install anywhere via `pip`. 4. Testable (maybe via `pytest` and `hypothesis`, as @aarchiba has tried to do?). 5. Extendible (for instance, other implementations of the algorithm could go here?). Currently, the package provides a single method, also called `fftfit`, that takes in a profile and a template and returns the output of the FFTFIT algorithm. It installs and works on my machine (which runs Arch Linux), but I still haven't be able to test it on other OSes. I am working on adding the ability to automatically build the wheels, via `cibuildwheel` and GitHub Actions, so that they can then be released on PyPI. If anyone would like to help me with anything, feel free to open an issue or a pull request 👍🏾 !
matteobachetti commented 2 years ago

@astrogewgaw thanks for this work, it's extremely useful!

However, it is not working as expected, and fails in a seemingly simple case. I opened an Issue in the repo: https://github.com/astrogewgaw/fftfit/issues/1

astrogewgaw commented 2 years ago

@matteobachetti Thanks for pointing this out 👍🏾 ! It does seem a bit weird, since the code has not been altered in any way, and it has been linked almost exactly the way it's done in PRESTO. I will look into it as I get time and let y'all know how it goes.

PS: Thanks for opening the issue, and that too with a MWE 😁 👍🏾 ! That's going to be quite helpful.