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

Fix race conditions in differential evolution #1063

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Fix race conditions in differential evolution #1063

merged 1 commit into from
Jan 17, 2024

Conversation

NAThompson
Copy link
Collaborator

@NAThompson NAThompson commented Jan 9, 2024

Through a combination of silly mistakes, I missed a pile of race conditions in the OpenMP threading.

Switch to C++ threading. Note that this change requires serial generation of trial vectors.

Hopefully I can figure out to parallelize the generation of trial vectors to reduce the serial section a la Ahmdahl's law,
while simultaneously keeping thread sanitizer happy.

@NAThompson NAThompson force-pushed the jso branch 5 times, most recently from fdbb506 to fca35ef Compare January 9, 2024 06:16
@NAThompson
Copy link
Collaborator Author

NAThompson commented Jan 9, 2024

@jzmaddock : I wonder if we could revisit the "no unicode in comments" rule-I had to remove all the accents from the author's names which I found somewhat inelegant. Going a bit further, maybe something like:

template<typename ℝ>

could really improve readability. I know we can use // boost-no-inspect, but the other inspection rules are quite valuable.

@mborland
Copy link
Member

@jzmaddock : I wonder if we could revisit the "no unicode in comments" rule-I had to remove all the accents from the author's names which I found somewhat inelegant. Going a bit further, maybe something like:

template<typename ℝ>

could really improve readability. I know we can use // boost-no-inspect, but the other inspection rules are quite valuable.

My two cents is I think revisiting the no unicode in comments would be good, but adding unicode into the actual code is not. SO says it varies from formerly illegal to just unsupported now: https://stackoverflow.com/questions/26660180/are-unicode-and-special-characters-in-variable-names-in-clang-not-allowed.

@jzmaddock
Copy link
Collaborator

I don't know if it's still the case (but I assume it is) that users on Windows in far eastern countries do not use Unicode character sets, and Unicode in source files (even in comments) renders them non-compilable. If you search hard enough you'll find some old bug reports on this which crop up whenever Unicode has crept in accidentally. Usually been my fault in the past, and usually for authors names as in your case here. Whatever we should run this past the mailing list rather than making a unilateral change?

@NAThompson
Copy link
Collaborator Author

@jzmaddock , @mborland : I do enjoy the code readability improvements that unicode can bring, but it seems like this is an ax I probably shouldn't grind . . .

@NAThompson NAThompson force-pushed the jso branch 19 times, most recently from dea8670 to 67cabce Compare January 15, 2024 22:01
@NAThompson NAThompson force-pushed the jso branch 4 times, most recently from 06c6ad9 to e655c8a Compare January 16, 2024 21:43
@NAThompson NAThompson changed the title JSO Fix race conditions in differential evolution Jan 16, 2024
@NAThompson NAThompson marked this pull request as ready for review January 16, 2024 21:48
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 115 lines in your changes are missing coverage. Please review.

Comparison is base (79b4015) 85.31% compared to head (da1fdc7) 85.22%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1063      +/-   ##
===========================================
- Coverage    85.31%   85.22%   -0.10%     
===========================================
  Files          873      875       +2     
  Lines        66532    66568      +36     
===========================================
- Hits         56765    56733      -32     
- Misses        9767     9835      +68     
Files Coverage Δ
test/test_functions_for_optimization.hpp 100.00% <100.00%> (ø)
test/math_unit_test.hpp 24.00% <33.33%> (+0.50%) ⬆️
...boost/math/optimization/differential_evolution.hpp 82.00% <82.00%> (ø)
test/differential_evolution_test.cpp 59.70% <55.93%> (-40.30%) ⬇️
include/boost/math/optimization/detail/common.hpp 26.96% <26.96%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79b4015...da1fdc7. Read the comment docs.

Copy link
Member

@mborland mborland left a comment

Choose a reason for hiding this comment

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

TSAN and Helgrind both seem happy now. My one concern is that codecov shows that all the regions that involve synchronization are not covered by tests. Is this due to GHA not having enough threads available? It would probably be good to cover all of your param checks, and testing of validating your initial guess.

Through a combination of silly mistakes, I missed a pile of race conditions in the OpenMP threading.

Switch to C++ threading. Note that this change requires serial generation of trial vectors.

Hopefully I can figure out to parallelize the generation of trial vectors to reduce the serial section a la Ahmdahl's law,
while simultaneously keeping thread sanitizer happy.
@NAThompson NAThompson merged commit de9a1a0 into develop Jan 17, 2024
67 of 70 checks passed
@NAThompson NAThompson deleted the jso branch January 17, 2024 18:20
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.

3 participants