snoplusuk / echidna

MIT License
4 stars 12 forks source link

Config V2.0 #76

Closed jwaterfield closed 9 years ago

jwaterfield commented 9 years ago

New version of echidna which uses config files to specify dimensions to allow users to define their own variables. Supersedes #68

jwaterfield commented 9 years ago

Hey @ashleyrback I still need to add documentation but the code should all be there if you wanted to start reviewing. Cheers.

jwaterfield commented 9 years ago

@ashleyrback just realised plot_root needs converting still to.

ashleyrback commented 9 years ago

@jwaterfield, apologies for the delay, haven't had much in the way of a reliable internet connection. The code all looks really good - all the logic seems to make sense. There are lots of errors when I try to build the docs (from your branch) though. Most of them seem to be ImportErrors relating to imports from echidna.output.store or echidna.output.plot.

Also the unittests currently fail with three errors:

$ python -m unittest discover echidna/test/
......E...E.....E
======================================================================
ERROR: test_get_chi_squared (test_chi_squared.TestChiSquared)
Tests get chi squared method
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_chi_squared.py", line 114, in test_get_chi_squared
    mc_spectrum)
  File "echidna/limit/chi_squared.py", line 90, in get_chi_squared
    if (kwargs.get("penalty_terms") is not None):
NameError: global name 'kwargs' is not defined

======================================================================
ERROR: test_store (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_store
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name
    __import__(name)
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_store.py", line 3, in <module>
    import echidna.output.store as store
  File "echidna/output/store.py", line 4, in <module>
    import echidna.limit.limit_setting as limit_setting
  File "echidna/limit/limit_setting.py", line 7, in <module>
    import echidna.output.plot_root as plot
  File "echidna/output/plot_root.py", line 2, in <module>
    from echindna.output import root_style
  File "/home/ashley/snoing/install/root-5.34.30/lib/ROOT.py", line 352, in _importhook
    return _orig_ihook( name, *args, **kwds )
ImportError: No module named echindna.output

======================================================================
ERROR: test_limit_setting (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_limit_setting
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name
    __import__(name)
  File "/home/ashley/snoing/install/root-5.34.30/lib/ROOT.py", line 352, in _importhook
    return _orig_ihook( name, *args, **kwds )
  File "/home/ashley/snoplus/software/echidna/echidna-pr/echidna/test/test_limit_setting.py", line 7, in <module>
    import echidna.limit.limit_setting as limit_setting
  File "/home/ashley/snoing/install/root-5.34.30/lib/ROOT.py", line 352, in _importhook
    return _orig_ihook( name, *args, **kwds )
  File "echidna/limit/limit_setting.py", line 7, in <module>
    import echidna.output.plot_root as plot
  File "/home/ashley/snoing/install/root-5.34.30/lib/ROOT.py", line 352, in _importhook
    return _orig_ihook( name, *args, **kwds )
  File "echidna/output/plot_root.py", line 2, in <module>
    from echindna.output import root_style
  File "/home/ashley/snoing/install/root-5.34.30/lib/ROOT.py", line 352, in _importhook
    return _orig_ihook( name, *args, **kwds )
ImportError: No module named echindna.output

----------------------------------------------------------------------
Ran 17 tests in 9.897s

FAILED (errors=3)

I'll try and keep an eye onthis.

ashleyrback commented 9 years ago

@jwaterfield sorry didn't mean to close it, pressed the wrong button.

ashleyrback commented 9 years ago

@jwaterfield, all unittests pass now. Still some problems building the docs which I've commented on. Also want to run some of the example scripts to see how these work with config files.

jwaterfield commented 9 years ago

Thats the way the bipo cuts work currently. The default is not to have the bipo cuts i.e. args.bipo is Fasle by default (done explicitly a few lines below L110). To switch them on a user would need to have the command --bipo in the command line. --no-bipo does the same as not including --bipo in the command line but I added it so a user can be explicit with their commands if they wanted to. I've added a note in the help of argparse to make it clearer to the user that bipo cuts are not applied by default. See next commit

ashleyrback commented 9 years ago

@jwaterfield can you let me know when you've pushed the args. changes and fixed the error message. Cheers

ashleyrback commented 9 years ago

Sorry just noticed you fixed the args., can you just let me know when you've fixed the unexpected keyword argument error. Cheers

ashleyrback commented 9 years ago

Cheers, looks like it's working now. I'll just finish re-creating the Majoron spectra and testing my the Majoron limits script, then it should be good to merge.

ashleyrback commented 9 years ago

multi_ntuple_spectrum is now working perfectly for my Majoron spectra :+1:

ashleyrback commented 9 years ago

@jwaterfield thanks for making those changes. I'm just smearing my Majoron spectra to check the majoron limits script. Two are finished but three jobs still going. As soon as it's done I'll post the results and should be fine to merge.

ashleyrback commented 9 years ago

Since it is a major change, here is the verification document for this PR pr_76_verification.pdf. As stated in the document, happy to merge, based on the following conditions:

ashleyrback commented 9 years ago

Merging!