Cool!

I can have a look in a few days, just one comment about this:

The input to the statistic functions is func(measured_values, evaluated_values, weights)

Is it possible to have these inputs to the fit statistic more flexible?

One of the fit statistics I'd like to implement in astropy that is common in X-ray and gamma-ray astronomy is wstat:
http://heasarc.nasa.gov/xanadu/xspec/manual/XSappendixStatistics.html

It doesn't take "weights" as input, like chi^2 statistics do, but instead a few other quantities.

Here's an example where @registerrier implemented wstat as a user-defined statistic in Sherpa:
https://gist.github.com/cdeil/8026184

It would be nice if the astropy.modeling API for fit statistics were more general so that such hacks are not needed to implement more complex fit statistics like wstat.

If this is already the case, apologies.
I'll take a closer look at the API that you propose here and will try implement wstat as an example in a few days.

Thanks for doing this!

The only hard requirement is that statistic is callable.
However, if it is in the above form, fitters can use the base class Fitter.errorfunc. Otherwise they have to implement their own errorfunc to match the interface of the statistic.

This can probably be improved but I am putting it out for discussion. It'd be great to have more examples, looking forward to the wstat example.

The current Travis failure is an example in whatsnew/0.3.rst, failing due to a name change. The proposed name change, if accepted, is going to be in 0.4 (NonLinearLSQFitter -- > LevMarLSQFitter) while the failing example is for 0.3.

Is there a way to skip a test based on the version of astropy?
Or perhaps this should be skipped entirely.

@nden I don't know how to get rid of the travis-ci test failure you mentioned in the what's new example. Maybe @astrofrog knows or you can ask on the astropy-dev list ... this will probably be a common problem as astropy ages...

@cdeil I just pushed an update which I hope will make it easier to add a wstat statistic. Can you confirm this? I need to work some momre on this but wanted to give you an idea.

I'll try this out now ... maybe @astrofrog and / or @embray, you can have a look, too?
This is a major improvement that has been lying around for two months and it should go into master soon so that it gets some real-world testing before the 0.4 release.

@nden Can you please add an entry to the CHANGES document?
(I think that's what we should do for modeling for 0.4 ... note major changes, but not start adding deprecation warnings)

It's not quite ready for review. I just want to make sure it's flexible enough to add other statistic functions and optimizers.

I don't know if there's much sense in working on new features until #2049 is resolved since everything in modeling depends on how parameters are handled but @embray is working on it.

The fit statistics functions (leastsquare, cash and chi_line) need docstrings.

I have re-implemented all build-in fit statistics from Sherpa in gammapy.stats.fit_statistics, see e.g. gammapy.stats.fit_statistics.cash for an example.

Some points for discussion about the fit statistics functions:

• Should fit statistics return the sum over bins or the per-bin fit statistic? (Sherpa returns both I think)
• Do we need the chi_line fit statistic in addition to the leastsquare fit statistic? (see the various chi_square fit statistics in Sherpa / gammapy)
• Can we avoid having special fit statistics for 1-dim, 2-dim, ... data? (If yes, there should be no fit statistic with name *_line)

@nden I wasn't aware that #2049 is the bottleneck at the moment ... in this case I'll continue looking at this later.

Just one more comment ... I suggest we rename Fitter.errorfunc to Fitter.fit_statistic or Fitter.objective_function, which are more general terms ... I think errorfunc is least-squares-specific terminology and I find it confusing for the general Fitter API.

All good questions.
I would prefer not to have special statistics but it's not clear to me that chi_line can be avoided and it's a useful case. I hope someone can prove me wrong.

I am in favor of objective_function since this is what's generally used. fit_statistic is easily confused with the statistic functions used within the objective function.

@cdeil After mulling over this for a while, I decided it's best to limit this PR to only separating statistics from optimizers and add additional statistics in separate PRs.
Outside this PR I tested that it works with other statistic types so I think it's ready for others to try to add their favourite statistic. I am marking it as ready for final review. There are name changes in this PR so I will add entries in CHANGES.rst before merging after everyone had a chance to change the names again.

This got stuck for some reason. There are so many changes in upstream/master that even rebasing or manually merging upstream locally does not produce diffs that are easy to review. So I prepared a new branch based on the current trunk. I am wondering if I replace the current branch with the new branch with "git branch -M ..." would Travis pick up the new branch? Or is it better to make a separate PR?

Ah, I see. The goal was to make it possible to write a statistic without having to modify the objective function, defined in the fitter.

Adding fitter.statistic and fitter.optimizer properties will allow changing them after a fitter is initialized, for example:

fitter = fitting.SimplexLSQFitter()
fitter.statistic = cash

This is easy to add.

It may be good also to add a function (custom_fitter) which takes an optimizer and a statistic and returns a fitter. This will take some more work.

@taldcroft Another option is to pass the statistic to fitter.__call__ . I prefer the properties though. Do you see any reasons this would be better than having properties?

@nden @taldcroft @cdeil - just to check, what is the current status of this? I think we had planned to re-apply the changes rather than trying to rebase? If so, would it make sense to do this this week so we can merge it (or have it ready for review) before the feature freeze?

My understanding is that this, as well as #2149 and #1985 should be closed as they are part of the refactoring of modeling that @embray is doing.

I would like to keep all of these open as reference--all of them address issues that should be tracked regardless of whether any attached implementations are used as-is.

Actually, were there still any major lingering issues with this? I know there were some minor niggles but they could be addressed later. In the interest of getting some of this functionality sooner rather than later I can rebase this to merge into the current master if people thing that would be better.

I've completely lost track with the many open astropy.modeling issues and pull requests.
Separating optimisation from fit statistic is in my opinion a big step in the right direction.
So 👍 to merge some of them into master now and then re-evaluate if everything works well or some things need to be adapted.

Something that needs to be added here is a simple way for a user to change the statistic function (See Tom's comments above). I did some work in this direction but it wasn't completed.

Okay, I've rebased this on master in a local branch of my own and ensured all the tests are passing on all supported Python versions. I think I'd rather go ahead and merge this as-is, and then build any additional enhancements on top of what's in there.

The only thing this is lacking is a changelog entry, and perhaps also warrants a mention in the 0.4 what's new page.

Unless you don't want to advertise it too broadly if the feature is considered "incomplete". But there are still API changes that deserve mention.

I think we can work on the 'what's new' after feature freeze, so I'd just add the CHANGES.rst entry for now, and we can worry about the 'what's new' after.

Okay...I just worry about things slipping through the cracks with that. I wasn't really following if people were writing that or not as is.

@embray - I just created #2416. Note that if you want to write it now, you are very welcome to! But if there are enough other things to worry about today for the feature freeze, this can be delayed.

Okay thanks, sounds good. Will have to go through and find out what else needs to be in there. Need to ping other devs too.

Will ping some devs now on the gitter channel (since a few are hanging out there)

@nden @astrofrog @embray - what's the story with this? Can it be rebased and merged for 0.4? Or do we need to push it off?

Closed by #2441