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

Add Poisson likelihood fitter #1056

Closed
3 tasks
cdeil opened this issue May 6, 2013 · 9 comments
Closed
3 tasks

Add Poisson likelihood fitter #1056

cdeil opened this issue May 6, 2013 · 9 comments

Comments

@cdeil
Copy link
Member

cdeil commented May 6, 2013

I would like to add a PoissonLikelihoodFitter as available e.g. in Sherpa (described here) to be able to fit Poisson count data.

A preliminary version can be found here: class, example.

TODO:

  • Make it work with n-dimensional models and data (currently only supports 1-dimensional)
  • Pass extra arguments along to scipy.optimize.minimize.
  • Is this the right way to handle integral models?

cc @nden, @embray Should I try to implement this now and get it into astropy or should I wait for your modeling APE and further structural changes in astropy.modeling?

EDIT: the issue description was updated, so some comments below are outdated / irrelevant now.

@eteq
Copy link
Member

eteq commented May 6, 2013

I don't know if we ever fully specified a version. It's already an "optional" dependency, so using whatever version makes sense is fine.

Probably the best thing to do is a try/except inside the function that explicitly states something like "Scipy 0.11 is needed for custom fit functions"

@cdeil
Copy link
Member Author

cdeil commented Oct 19, 2013

Here's a first prototype version: http://nbviewer.ipython.org/7062154
that I managed to write thanks to @nden's example here: https://gist.github.com/nden/5466946

@nden At the moment to create a user-defined fitter one has to add to the constraintsdef dict (or the Fitter initialisation will fail in the _constraintscheck):

from astropy.modeling.fitting import constraintsdef
constraintsdef['CashFitter'] = ['fixed']

I guess this shouldn't be needed?
If it is, it should be described here.

@embray
Copy link
Member

embray commented Oct 23, 2013

Nice. I do think there are some areas where defining a new fitter could be easier. I have a note somewhere, for example, that the constraintsdef dict shouldn't be used explicitly anywhere, and that the purpose it serves could be handled automatically by a Fitter metaclass and a couple class attributes. We'll see if I get to that. Defining the errorfunc could be a bit easier too. But overall it looks like that was pretty easy.

@cdeil
Copy link
Member Author

cdeil commented Oct 24, 2013

+1 to removing the global constraintsdef dict and storing this info in classes.

@embray
Copy link
Member

embray commented Oct 24, 2013

Done in 3552d36

@astrofrog astrofrog modified the milestones: v1.0.0, v0.4.0 May 3, 2014
@astrofrog
Copy link
Member

+1 to the idea, but removing 1.0 milestone

@cdeil
Copy link
Member Author

cdeil commented Mar 2, 2016

We're using Sherpa now, so we don't need this in Astropy any more and I don't plan to work on it there.
Closing this old issue.

@cdeil cdeil closed this as completed Mar 2, 2016
@embray
Copy link
Member

embray commented Mar 2, 2016

Would rather leave issues like this open--just because you don't need it or plan on working on it for Astropy doesn't mean it's not a good issue to keep open.

@embray embray reopened this Mar 2, 2016
@cdeil cdeil changed the title WIP: Add Poisson likelihood fitter Add Poisson likelihood fitter Mar 2, 2016
@cdeil
Copy link
Member Author

cdeil commented Mar 2, 2016

An issue is mainly useful if there's a chance that it leads to improvements, right?

If it's just a bunch of year-old and outdated discussion keeping it open and making people re-read through it is just annoying.

In this case if someone is interested to implement this in astropy.modeling, they should ignore the discussion here and continue with #3786 .

@pllim pllim added the Close? label Mar 2, 2016
@cdeil cdeil closed this as completed Mar 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants