Skip to content

Conversation

@knoepfel
Copy link
Member

This PR fixes a data race in filtering, where a filter result was being sent multiple times for the same data cell. This PR also introduces unit tests that verify the expected filtering behavior.

@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
phlex/core/filter.cpp 0.00% 0 Missing and 3 partials ⚠️

❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project check has failed because the head coverage (56.83%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
- Coverage   57.05%   56.83%   -0.23%     
==========================================
  Files         116      116              
  Lines        2580     2583       +3     
  Branches     1104     1105       +1     
==========================================
- Hits         1472     1468       -4     
- Misses        276      280       +4     
- Partials      832      835       +3     
Flag Coverage Δ
unittests 56.83% <50.00%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
phlex/core/detail/filter_impl.cpp 58.49% <100.00%> (+1.62%) ⬆️
phlex/core/detail/filter_impl.hpp 80.00% <ø> (-20.00%) ⬇️
phlex/core/filter.cpp 38.88% <0.00%> (-6.40%) ⬇️

... and 2 files 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 e33a01a...7c76b49. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@greenc-FNAL
Copy link
Contributor

This isn't the cause of the occasional test failure, is it?

@knoepfel
Copy link
Member Author

This isn't the cause of the occasional test failure, is it?

Maybe. I know at least one of the benchmark tests that was failing (benchmark:09) was using filters. I haven't seen it fail with this fix. But I want to test it on scisoftbuild02; I've been testing on my Mac.

@knoepfel
Copy link
Member Author

Maybe. I know at least one of the benchmark tests that was failing (benchmark:09) was using filters. I haven't seen it fail with this fix. But I want to test it on scisoftbuild02; I've been testing on my Mac.

I've tested this on scisoftbuild02 repeatedly, and not encountered any failures.

Copy link
Contributor

@wddgit wddgit left a comment

Choose a reason for hiding this comment

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

Looks like it fixes the bug. There was one minor suggestion, but that is all I noticed.

Add filtering unit-test checks to verify expected behavior
@knoepfel knoepfel merged commit b99d7a0 into Framework-R-D:main Oct 17, 2025
9 of 12 checks passed
@knoepfel knoepfel deleted the fix-filtering branch October 17, 2025 20:28
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