-
Notifications
You must be signed in to change notification settings - Fork 60
Suggestions from pypi PR #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Thank you guys. So I'm merging and will now proceed with the remaining PRs. Have a nice week-end.
Thanks @vvolkl I'm merging and will now send the final PR, restructuring for pypi.
Merci David!
latest fix for OCNDOR with old setup : it works, new setup needs more…
cbernet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, please see my comments, I think some discussion is needed for a couple of them.
heppy/test/test_gun.py
Outdated
|
|
||
| def setUp(self): | ||
| random.seed(0xdeadbeef) | ||
| #random.seed(0xdeadbeef) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to get reproducible results. why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test was failing for me because a temporary directory the test wanted to create using mkdtemp already existed. I was just checking if that was the result of using the same seed, but I see now that it's unrelated. Will remove
| mean, sigma = plot(rootfile) | ||
| self.assertAlmostEqual(mean, 110.87, 1) | ||
| self.assertAlmostEqual(sigma, 18.6, 1) | ||
| self.assertLess(abs(mean- 110.87), 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sounds dangerous to me. these tests are here to make sure that changes to papas that are not supposed to change the physics (software changes) indeed do not change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have mentioned, three tests are failing right now for me (see below) so it seems papas already went of course. I'm just trying to get the test suite to run since some tests are better than no tests. It seems that the result of test_analysis_ee_Z.TestAnalysis_ee_Z and test_analysis_ee_ZH_mumubb is still reasonable ( 108.75454735874334 vs 110.87 ) so I just decreased the sensitivity.
======================================================================
FAIL: test_analysis_ee_ZH_mumubb (test_analysis_ee_ZH.TestAnalysis_ee_ZH)
Check for an almost perfect match with reference.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/opt/fcc/repo/heppy/heppy/test/test_analysis_ee_ZH.py", line 65, in test_analysis_ee_ZH_mumubb
self.assertAlmostEqual(mean, 110.87, 1)
AssertionError: 108.75454735874334 != 110.87 within 1 places
======================================================================
FAIL: test_z_clic (test_analysis_pdebug_clic.TestAnalysis_ee_Z)
Check Z mass in ee->Z->ddbar (CLIC).
----------------------------------------------------------------------
Traceback (most recent call last):
File "/opt/fcc/repo/heppy/heppy/test/test_analysis_pdebug_clic.py", line 62, in test_z_clic
self.assertEqual(0,os.system("source $HEPPY/test/data/pdebug_python_check.sh physics_clic.txt $HEPPY/test/data/required_clic_physics_dd.txt"))
AssertionError: 0 != 256
======================================================================
FAIL: test_z_2_clic (test_analysis_ee_Z.TestAnalysis_ee_Z)
Check Z mass in ee->Z->ddbar (CLIC).
----------------------------------------------------------------------
Traceback (most recent call last):
File "/opt/fcc/repo/heppy/heppy/test/test_analysis_ee_Z.py", line 110, in test_z_2_clic
self.assertAlmostEqual(mean, 90.15, 1)
AssertionError: 90.71223590370774 != 90.15 within 1 places
----------------------------------------------------------------------
Ran 125 tests in 153.827s
FAILED (failures=3)
|
|
||
|
|
||
| source /cvmfs/fcc.cern.ch/sw/views/releases/externals/94.1.0/x86_64-slc6-gcc62-opt/setup.sh | ||
| source /cvmfs/fcc.cern.ch/sw/views/releases/externals/94.2.0/x86_64-centos7-gcc62-opt/setup.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok!
Hi Colin,
find here some suggested changes corresponding to my comments (also the changes of two of my PR's are in here).
There is another issue: we decided to deprecated fcc-physics in one of the last meetings but I discovered it is still used for the JetClusterizer in heppy. I will move it to fcc-edm, where we also create a library + dictionary.
Installing proper executables with
setup.pyis trickier than I thought initially, mostly due to heppy_interactive. your current approach of writing a tmp_file also doesn't seem to work when you install heppy as a module, so I'm a bit out of my depth what a good solution could be!