Fit class

[ashleyrback]

Supersedes #111

Additionally adds:

Adds FitResults class, which acts as a container for the parmeter grid. Also contains a copy of the FitConfig class, so that it can assign the best fit values of each FitParameter, conce the fit has run.

Also adds the GridSearch class - which contains the original grid- search algorithm. This class inherits from FitResults as the _data attribute in FitResults is the grid that should be filled during the grid-search.

Also adds the _funct method to Fit, which is the begginnings of the callable that should be passed to any optimiser.

[ashleyrback]

@jwaterfield I've just merged in the latest version of your PR and pushed my GridSearch and FitResults stuff, so we can see everything together.

I'mg going to go through now and check everything fits together OK. Expect quite a few questions.

[ashleyrback]

@jwaterfield I've pushed all the latest updates to the fitting code, including adding content to the Fit._funct method and adding the ability to load pre-convolved spectra from file. Let me know if there is anything missing or that you think should be changed. I'm going to write some simple examples/tests for it now.

[ashleyrback]

I've merged mine and @jwaterfield's updates from Friday and pushed them to this PR.

@jwaterfield, there were a couple of bugs that I've noted in the comments above.

@EdLeming, as soon as the bugs mentioned above are fixed, you can start checking the PR for documentation, pep8 and current unittests.

And then you'll just have to check with the additional unittest for the Fit class, once we've written it. Plus confirm that we've checked agreement with previous limits.

[ashleyrback]

@EdLeming, all the comments have now been addressed, so it should be fine for you to start reviewing this PR for documentation, existing unittests and pep8. Please refer to this guide on the wiki and keep us up to date using labels to mark if changes to documentation are required or if unittests fail.

Myself and @jwaterfield will continue to work on verifying limits against a previous tagged release and adding further unittests.

It would be really good if we can try and get this merged by the double beta call next week, i.e. just under a week from now! Then James and I can present the updates and verification on the call.

We can chat on Skype if needed, just let me know.

[EdLeming]

I get ~20 tests failing with the below error.

File "echidna/core/spectra.py", line 802, in init super(SpectraConfig, self).init("spectra") TypeError: init() takes exactly 3 arguments (2 given)

[jwaterfield]

@EdLeming unittests except one are fixed on my global_fit_config branch which @ashleyrback needs to merge into this PR. The unittest that is failing is the following which I believe @ashleyrback added so if you can fix this final one Ashley that would be great!

ERROR: test_minimise (unittest.loader.ModuleImportFailure)

ImportError: Failed to import test module: test_minimise Traceback (most recent call last): File "/cm/shared/apps/python/2.7.10/lib/python2.7/unittest/loader.py", line 254, in _find_tests module = self._get_module_from_name(name) File "/cm/shared/apps/python/2.7.10/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name import(name) File "/lustre/scratch/epp/neutrino/jw419/echidna_env27/echidna_pr/echidna/echidna/test/test_minimise.py", line 6, in from echidna.limit.minimise import GridSearch ImportError: No module named minimise

---

[ashleyrback]

@EdLeming and @jwaterfield, yep I'm fixing that error at the moment. There were a couple of additional problems too. I'll merge in James' changes and then push everything at the same time.

[EdLeming]

Sorry, I haven't had a single chunk of time to commit to this today so you'll likely get these issues in dribs and drabs. Minor pep8 issues in :

echidna/config/fit_example.yml echidna/config/grid_search_test.yml echidna/errors/custom_errors.py echidna/scripts/tutorials/getting_started.py

Mostly just multiple statements on the same line, but a few blank spaces / lines too long. I don't know how keen you are on removing absolutely all issues.

[ashleyrback]

I've pushed mine and James' changes, that should fix the unittests.

[EdLeming]

Yep. All passes fine now.

[ashleyrback]

@EdLeming I don't think we need to pep8 check .yml files. Pep8 is only really a python standard, so should certainly be applied to all .py files, but YAML is not a native python standard, so we probably shouldn't be too concerned about applying pep8 to these files.

[ashleyrback]

I'll check the other two.

[ashleyrback]

@EdLeming, that should fix the pep8 errors for those other two files.

[ashleyrback]

@EdLeming, sorry I kept tweaking things whilst analysing and adding these to the PR. I'm happy with it now though. There are probably still several places where there is documentation missing, so I'll check through those in the next couple of days.

Are you still OK to review it?

[EdLeming]

Still OK. Just let me know when you're ready and I'll look at it for you.

Cheers, Ed

On Tue, Feb 2, 2016 at 5:26 PM, ashleyrback notifications@github.com wrote:

@EdLeming https://github.com/EdLeming, sorry I kept tweaking things whilst analysing and adding these to the PR. I'm happy with it now though. There are probably still several places where there is documentation missing, so I'll check through those in the next couple of days.

Are you still OK to review it?

— Reply to this email directly or view it on GitHub https://github.com/snoplusuk/echidna/pull/114#issuecomment-178700625.

[ashleyrback]

I've uploaded my plot showing my current best agreement with KamLAND-Zen's results. There is still some tweaking to do with the n=7 case but it is a vastly better agreement than I was able to get with the previous tagged release of echidna. So this is my confirmation that I'm happy with this version, from the point-of-view of verification against KamLAND-Zen.

I think @jwaterfield has also verified this version for his analysis.

[ashleyrback]

@EdLeming I think it is ready to look at now. I just pushed my latest commits. Like I said there is probably some documentation missing and it would probably benefit from one or two additional unittests, but all the code is there. So, if you've got time, feel free to start reviewing it from now and let me know if there are any problems. Cheers.

[EdLeming]

OK, if I haven't got back to you by Friday, remind me. Lets get it done by the end of the week.

[ashleyrback]

@EdLeming just pushed one more commit which allows you to disable a parameter's penalty term completely by explicitly setting sigma to None.

[ashleyrback]

@EdLeming, added another small bit of functionality that I need for my analysis. Let me know if you need any further help with the review.

[EdLeming]

Sorry, Ashley. I'm in Oxford until Wednesday. I promise I'll crack through this then

[ashleyrback]

No worries, there might be a small commit to add to fix #130. Also I noticed today that about three unittests currently fail so I'll try and fix those tomorrow.

[mjmottram]

@ashleyrback is there an example script that works with the fit class?

[ashleyrback]

@EdLeming unittests now fixed. I'll address @mjmottram's two comments and I got a couple of errors when building the docs, so I'll check that too.

Also, the issue of overcrowding of the spectra module came up in conversation with Matt this morning, I think I will go ahead and move Config and derived classes to a config module (still in core) and Parameter and derived classes to a parameter module. Does this sound OK?

[EdLeming]

Sounds sensible. I've been becoming more and more convinced that the spectra module was too crowded. I've found myself looking in there a lot, but only ever (really) for spectra specific stuff

[ashleyrback]

@EdLeming, I've made the following changes:

• Addressed Matt's comments.
• Separated the Config and Parameter classes out of the spectra module
• Tidied up the limit module, and separated it out into fit and limit.

My tidying up does seem to have caused conflicts with master now - I think probably because of moving and deleting files, though I thought git should be able to cope with this.

@mjmottram I definitely agree with your proposal about setting up a development branch for larger PRs like this.

@snoplusuk/core, please read the full commit message for "Tidies spectra and limit modules. Are you happy for me to delete things such as the old limit_setting, limit_config and chi_squared modules, or should we keep these in for now but raise a DepreciationWarning?

There are a couple of issues that need to be solved, but I think we can merge this PR on the condition that they will be resolved shortly after. They are:

• Update zero_nu_limit #131
• Replace plot_chi_squared #132
• Add unittest or other test for Fit and Limit classes #133
• Add higher level example/tutorial of fitting/limit setting #134

Is there anything else we need to do to get this merged?

[ashleyrback]

@EdLeming I've fixed the conflicts! It was because I edited the getting_started tutorial, but hadn't merged in the changes I made recently in a separate PR. All fixed now, can be merged automatically :smile:

Let me know if there is anything else that needs to be changed, but I'm pretty sure it is ready to merge now.

[EdLeming]

Ok. So this passes all the tests and the major changes look good.

If I was going to test all of this it would literally take days, but I think I'm happy I follow all the major functionality. I do plan to continue testing this version in the coming days, but I think I'm happy that it basically works. At this point I think it's better to merge this and flag anything we notice in future as issues.

If you're happy with that philosophy I'll go ahead and merge.

[ashleyrback]

Yes, I'm happy with that philosophy. I think we'll all be testing it over the next few days, so if you get any errors or spot any problems make an issue (and assign someone to it - preferably check with them first that they are happy to be assigned!).

Thanks @EdLeming and @mjmottram for all your help testing and reviewing this.