-
Notifications
You must be signed in to change notification settings - Fork 101
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
Comments
@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. |
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. |
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. |
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. |
The python version is only a handful of lines. Is there any reason not to pull it into PINT? |
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? |
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. |
I think that would be great! |
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. |
@aarchiba, totally agree. We need to create an fftfit module with methods mimicking PRESTO's (e.g. |
I'd like to get this dealt with. Matteo, where is the fftfit code in HENDRICS? |
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 |
Great! Since @scottransom and Thankful are working on event_optimize, maybe they can incorporate this into PINT so we can remove that dependency! |
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. |
Wonderful, Anne! That would be great! |
I agree! I definitely trust your Opionions, @aarchiba! Thanks! |
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 But one Opinion in particular is maybe relevant to Getting really reliable uncertainties, even for synthetic data, is the real kicker, of course. |
@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 ;) ). |
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. |
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 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:
Currently, the package provides a single method, also called |
@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 |
@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. |
Currently the PINT script
event_optimize
requires thefftfit
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
The text was updated successfully, but these errors were encountered: