search

next-exp / nexus

Geant4 simulation framework of the NEXT Collaboration
5 stars 55 forks source link

Add test that runs the overlap check on the main geometries. #231

Closed paolafer closed 7 months ago

paolafer commented 7 months ago

This PR adds the overlap checks to the automatic tests for the main geometries. The main modification is the addition of a new exception class, which is custom-made with the warnings turned into exceptions. A new command line flag is also added to execute the check before running any events.

paolafer commented 7 months ago

The tests are actually failing because there is an overlap in the NEXT100 geometry (so the tests are working!). After the merging of PR #222 they shouldn't be failing anymore, therefore the approval of this PR will be done after the other one.

jwaiton commented 7 months ago

Now that PR #222 is merged, I've read through the code and I see no clear issues with it. However, I'm still having issues from pytest, and github seems unable to run said tests as well.

The error that I appear to be getting is:

============================================= short test summary info ============================================== FAILED tests/pytest/macros_test.py::test_run_overlap_check - subprocess.CalledProcessError: Command '['/home/e78368jw/Documents/NEXT_CODE/NEXUS/nexus/scripts/../bin/nexus',...

I've added a file more information from the test. fail_test.txt

It appears based on this line: Overlap with volume already placed ! Overlap is detected for volume KAPTON_BOARD:0 (G4Box) with SIPMSensl:0 (G4Box) overlap at local point (-0.580144,0.392996,0.175) by 150 um (max of 203 cases) NOTE: Reached maximum fixed number -1- of overlaps reports for this volume ! *** Fatal Exception *** core dump ***

Are there still overlaps occurring that are causing these tests to fail?

paolafer commented 7 months ago

Yes, I'm aware of that! It's an overlap in a different geometry, which @MiryamMV is taking care of. In fact, this PR has proven extremely useful because it detected this second overlap, too, which had been overseen. I think that the purpose of the PR has been accomplished, but we can wait to approve it after the modifications by @MiryamMV, in order not to have tests failing all the time!

jwaiton commented 7 months ago

Perfect, I'm glad to hear it is working as intended :smiley_cat:. I'll wait until I'm notified that said change has occurred before running it through again and approving.

As a side-note, it appears (although I'm likely mistaken) that the overlap tests increase the time of the pytests quite significantly (the github bot took 1h30m before reaching the overlapping geometries).

paolafer commented 7 months ago

Yes, I noticed the time issue, which is expected, because the precision for the overlap check has been increased w.r.t. the default one, to detect errors even in complex geometries. I'm trying to see if there's the possibility of adding a flag to decide if running the slow tests or not. It would be good to run them always before merging a PR, but maybe it's not necessary during debugging.

paolafer commented 7 months ago

I just rebased this branch on the updated master, with the remaining overlap fixed. I also added the slow mark to be able to run the tests without the slow ones. I would set the github workflow to skip the slow tests by default. However, they should be run by the reviewer before any PR approval.