-
Notifications
You must be signed in to change notification settings - Fork 11
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
Optimisation of the solid angle computation with aperture obstruction #715
base: devel
Are you sure you want to change the base?
Conversation
…ged them all into one
…e whole computation despite not being called
Thank you very much @obrejank , I'll fix the unit tests ( |
OK, the workflow is fixed, But, when running the full workflow (a 3x3 matrix with python https://github.com/ToFuProject/tofu/actions/runs/3620729344/jobs/6103424271 The error message suggests an issue with the compilation of I have checked the following :
Hence, this issue is specific to Any chance you could take a closer look into this MacOS-specifc issue @obrejank ? To edit the file that runs the unit tests on the testing matrix, you can play with the following file: |
I only had a quick look, but the final error messages says: |
If you look at the output in https://github.com/ToFuProject/tofu/actions/runs/3622194828/jobs/6106600138, you will see that MacOS uses clang when you call gcc: |
But the |
A possible fix could be to drop temporarily the support for |
So, I tried providing the correct argument: But it fails because the default https://github.com/ToFuProject/tofu/actions/runs/3622194828/jobs/6106600138
https://www.positioniseverything.net/clang-error-unsupported-option-fopenmp And apparently we have to make sure it's What I still don't understand is why it works on other branches... |
…d'environnement depuis l'interieur de tofu_helpers/openmp_helpers.py => lueur d'espoir ?
I was testing this a bit more extensively and found a bug that can be reproduced with the MWE script attached. It creates a set of sensors, with 2 apertures each. On this branch, the solid angle map becomes degenerate when the sensor becomes big, and even decreases instead of increasing, for some reason.
|
This PR is a follow up to #663 providing major performance improvement to all to the 3 functions
compute_solid_angle_apertures_light*
(which have also all been merged into one) thanks to:nogil
,Polygon
and even 3-5x faster than direct calls to GPC, the C library it reslies on)In my tests, using the Intel compiler, the speedup was about 60x in sequential (
OMP_NUM_THREADS=1
). The computation seems to be mostly memory-bound so the parallelisation efficiency is not very large (x3 with 8 threads), but it is still better than nothing and I'm not sure much more can be gained.The function
compute_solid_angle_apertures_visibility
could probably be merged in the same way but it is not called by the pipeline tests and I did not want to make changes I couldn't validate. Merging it with the rest would allow removing the current dependence to the python modulePolygon
.The CI pipeline fails because of the
matplotlib._contour
error, which is not related to my changes.