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

Dependency on fftfit #231

Open
paulray opened this issue Mar 7, 2017 · 23 comments · May be fixed by #777
Open

Dependency on fftfit #231

paulray opened this issue Mar 7, 2017 · 23 comments · May be fixed by #777

Comments

@paulray
Copy link
Member

paulray commented Mar 7, 2017

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Member Author

paulray commented Mar 9, 2017

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
Copy link
Member Author

paulray commented Mar 10, 2017

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.

@paulray paulray added this to the Complete photon data support milestone Mar 24, 2017
@aarchiba
Copy link
Contributor

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

@aarchiba
Copy link
Contributor

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
Copy link
Contributor

matteobachetti commented Aug 29, 2019

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
Copy link
Member Author

paulray commented Aug 29, 2019

I think that would be great!

@aarchiba
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Member Author

paulray commented Mar 6, 2020

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

@matteobachetti
Copy link
Contributor

matteobachetti commented Mar 6, 2020

@matteobachetti
Copy link
Contributor

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
Copy link
Member Author

paulray commented Aug 27, 2020

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

@aarchiba
Copy link
Contributor

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
Copy link
Member Author

paulray commented Aug 27, 2020

Wonderful, Anne! That would be great!

@scottransom
Copy link
Member

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

@aarchiba
Copy link
Contributor

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
Copy link
Contributor

@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 ;) ).

@aarchiba aarchiba linked a pull request Aug 28, 2020 that will close this issue
@kerrm
Copy link
Contributor

kerrm commented Dec 9, 2020

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:

  • template is always normalized by construction
  • support for analytic derivatives both with respect to parameters (e.g. gaussian no.1's width) and phase, i.e. the derivative of the template shape, which is useful for doing design matrices with the Poisson likelihood
  • caching of all of the above quantities, which sets a natural phase resolution

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
Copy link

Hey everyone 😁 ! Just wanted to let y'all know that, after talking to @scottransom (ref: scottransom/presto#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
Copy link
Contributor

@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: astrogewgaw/fftfit#1

@astrogewgaw
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants