snoplusuk / echidna

MIT License
4 stars 12 forks source link

Float on the fly #155

Closed jwaterfield closed 8 years ago

jwaterfield commented 8 years ago

Supersedes #152. Fixes conflicts and pep8

ashleyrback commented 8 years ago

I think we should change the inheritance of GridSearch. It now no longer needs to inherit from both FitResults and Minimiser, just Minimiser. This should mean that this line happens in when you call super in the line above.

ashleyrback commented 8 years ago

The docs fail to build correctly because of several warnings relating to:

ImportError: No module named rootStyle
jwaterfield commented 8 years ago

I would of thought Minimisers would still need to inherit from FitResults for the get_summary method?

ashleyrback commented 8 years ago

Also a couple of unittests need updating:

======================================================================
ERROR: test_shift (test_shift.TestShift)
Tests the variable shifting method.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_shift.py", line 102, in test_shift
    shifted_spectra = shifter.shift_by_bin(test_spectra, "energy_mc")
  File "echidna/core/shift.py", line 185, in shift_by_bin
    shifted_spec = spectra.Spectra(spectrum._name+"_shift" +
NameError: name 'spectra' is not defined

This looks like it is just missing an import.

======================================================================
ERROR: test_serialisation (test_store.TestStore)
Test saving and then reloading a test spectra.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_store.py", line 70, in test_serialisation
    store.dump("test.hdf5", test_spectra)
  File "echidna/output/store.py", line 100, in dump
    group.attrs["background_name"] = spectrum.get_background_name()
  File "echidna/core/spectra.py", line 221, in get_background_name
    % self._name)
ValueError: Couldn't construct background name of Test. This spectra must have its background name set manually.

I think you'll need to manually adjust the test spectrum and set its background name, then commit the updated spectrum.

======================================================================
ERROR: test_serialisation_fit_results (test_store.TestStore)
Test saving and then reloading :class:`FitResults`
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_store.py", line 162, in test_serialisation_fit_results
    fit_results.set_penalty_terms(penalty_terms)
AttributeError: 'FitResults' object has no attribute 'set_penalty_terms'

Here we want to test GridSearch and not FitResults so you should be able to do a straightforward replace.

======================================================================
ERROR: test_minimise (test_minimise.TestGridSearch)
Test the :meth:`GridSearch.minimise` method.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_minimise.py", line 194, in test_minimise
    self._test_statistic)
  File "echidna/fit/minimise.py", line 339, in minimise
    self.set_stat(result, tuple(indices))
  File "echidna/fit/minimise.py", line 540, in set_stat
    self._stats[indices] = stat
ValueError: setting an array element with a sequence.

----------------------------------------------------------------------

This one looks a little more tricky.

ashleyrback commented 8 years ago

Also there are three pep8 errors:

echidna/output/plot_root.py:9:1: E302 expected 2 blank lines, found 1
echidna/output/root_style.py:34:5: E265 block comment should start with '# '
echidna/fit/minimise.py:83:49: W291 trailing whitespace
ashleyrback commented 8 years ago

@jwaterfield, good point about the get_summary method, yes its probably worth keeping the dual inheritance for that.

ashleyrback commented 8 years ago

Can you remove this warning please, as reset_grids no longer exists in FitResults. And the same in the method below it.

ashleyrback commented 8 years ago

Also if possible (this will be needed for the unittest) can you add load/dump methods for GridSearch. These should be the same as for FitResults (or the old version of FitResults anyway).

ashleyrback commented 8 years ago

@jwaterfield all the previous pep8 errors are fixed but there are two new ones:

echidna/test/test_store.py:221:21: E126 continuation line over-indented for hanging indent
echidna/test/test_store.py:223:29: E126 continuation line over-indented for hanging indent

Other than that the changes look great and it's ready to merge once pep8 is fixed.

jwaterfield commented 8 years ago

@ashleyrback strange... That didnt come up on my pep8 checker. That should fix it though.

ashleyrback commented 8 years ago

Happy, merging!