Skip to content
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

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

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

paolafer
Copy link
Contributor

@paolafer paolafer commented Jan 9, 2024

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 paolafer requested a review from jwaiton January 9, 2024 15:11
@paolafer paolafer mentioned this pull request Jan 9, 2024
@paolafer
Copy link
Contributor Author

paolafer commented Jan 9, 2024

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.

@paolafer paolafer changed the title Add test that run the overlap check on the main geometries. Add test that runs the overlap check on the main geometries. Jan 9, 2024
@jwaiton
Copy link
Contributor

jwaiton commented Jan 22, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

jwaiton commented Jan 22, 2024

Perfect, I'm glad to hear it is working as intended 😺. 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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Contributor

@jwaiton jwaiton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds an overlap check to the tests for the main geometries, and includes a flag to disable the overlap checks to increase runtime (as the overlap checks are quite computing intensive). Good job @paolafer!

@paolafer paolafer merged commit 1d044d3 into next-exp:master Jan 24, 2024
1 check passed
@paolafer paolafer deleted the auto-overlap branch January 30, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants