snoplusuk / echidna

MIT License
4 stars 12 forks source link

Added function which copies spectra #73

Closed jwaterfield closed 9 years ago

jwaterfield commented 9 years ago

Thought of a more general way to do this will change this in my next commit

jwaterfield commented 9 years ago

@ashleyrback updated

ashleyrback commented 9 years ago

@jwaterfield looks good to me. I'll just check the unittests and check docs build OK.

ashleyrback commented 9 years ago

Passes all unittests. Just wondering if it would be useful to have a new unittest that checks to make sure the object returned is an exact copy of the original class instance. Maybe just looping through all class attributes to make sure they are the same, something like (in test_spectra.py):

for orig_value, copy_value in zip(original.__dict__.values(), copy.__dict__.values()):
    self.assertTrue(orig_value == copy_value)

may not work for _data but you could just check the results of sum().

jwaterfield commented 9 years ago

@ashleyrback Would you be able to check the unittest that I have committed? There is still ongoing python issues here (no unittest or discover modules)

jwaterfield commented 9 years ago

@ashleyrback typos should be fixed now.

ashleyrback commented 9 years ago

@jwaterfield Docs are fine now. Just a small point for future reference, if you put something like:

Returns:
  <return_type>: Description of what is returned.

The formatting comes out really nicely with a separate row for "Type". See latest version of decay module for examples.

unittest are just running now.

ashleyrback commented 9 years ago
.........E.......
======================================================================
ERROR: test_copy (test_spectra.TestSpectra)
Tests that the spectra are being copied correctly.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_spectra.py", line 192, in test_copy
    self.assertTrue(new_spectra._data == test_spectra._data)
  File "/usr/lib/python2.7/unittest/case.py", line 422, in assertTrue
    if not expr:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

----------------------------------------------------------------------
Ran 17 tests in 165.270s

This works:

self.assertTrue(new_spectra._data.all() == test_spectra._data.all())

Then with the above:

.........F.......
======================================================================
FAIL: test_copy (test_spectra.TestSpectra)
Tests that the spectra are being copied correctly.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_spectra.py", line 209, in test_copy
    self.assertTrue(new_spectra._name != test_spectra._name)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 17 tests in 170.424s

Not sure why this fails!

jwaterfield commented 9 years ago

All unittests now pass

ashleyrback commented 9 years ago

There are some pep8 errors, but mostly "line too long" errors where shortening/continuing on a new line, would make the code less readable. There are three that I think should be changed though:

In limit_setting:

echidna/limit/limit_setting.py:9:1: E302 expected 2 blank lines, found 1
echidna/limit/limit_setting.py:609:29: E126 continuation line over-indented for hanging indent
echidna/limit/limit_setting.py:686:1: E302 expected 2 blank lines, found 1

If you could just fix those three then it should be fine to merge. Cheers.

jwaterfield commented 9 years ago

@ashleyrback pep8 compliant now bar line length

ashleyrback commented 9 years ago

That's great. Cheers. Merging!