Skip to content

Fix Random Ray Solver Bug: Detect Multiple Point Sources in Same Subdivided Source Region #3471

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Jun 26, 2025

Problem

The Random Ray Solver had a silent bug where multiple point sources placed in the same subdivided source region would overwrite each other, with only the source having the higher index in the model::external_sources array being used. This could lead to incorrect simulation results without any warning to the user.

Root Cause

In FlatSourceDomain::convert_external_sources(), when mesh subdivision is enabled, point sources are mapped to a SourceRegionKey in the point_source_map_. The problematic line:

point_source_map_[key] = es;

This overwrites any existing entry for the same key, causing the silent loss of earlier point sources.

Solution

Added detection logic to identify when multiple point sources would map to the same SourceRegionKey and throw a clear error message instead of silently overwriting:

SourceRegionKey key {sr, mesh_bin};
auto it = point_source_map_.find(key);
if (it != point_source_map_.end()) {
  fatal_error("Multiple point sources detected in the same subdivided "
              "source region. This is not supported in the random ray "
              "solver.");
}
point_source_map_[key] = es;

Testing

The fix has been thoroughly tested with multiple scenarios:

  • Error Case: Multiple point sources at the same location correctly trigger the error
  • Valid Cases: Point sources at different locations continue to work normally
  • Non-Mesh Cases: Multiple point sources work correctly when mesh subdivision is disabled
  • Regression: All existing unit tests continue to pass

Benefits

  • Prevents silent data corruption and incorrect simulation results
  • Provides users with a clear, actionable error message
  • Maintains full backward compatibility for legitimate use cases
  • Follows the issue's guidance to provide an error rather than attempt complex feature support

The fix is minimal (5 lines of code) and surgical, addressing exactly the issue described while preserving all existing functionality.

Fixes #3470.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] Random Ray Solver Bug When Multiple Point Sources in Same Source Region Fix Random Ray Solver Bug: Detect Multiple Point Sources in Same Subdivided Source Region Jun 26, 2025
@Copilot Copilot AI requested a review from jtramm June 26, 2025 17:26
Copilot finished work on behalf of jtramm June 26, 2025 17:26
Copy link
Contributor

@jtramm jtramm left a comment

Choose a reason for hiding this comment

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

Looks good to me. Maybe change the wording slightly but otherwise this looks good.

@Copilot Copilot AI requested a review from jtramm June 26, 2025 18:33
Copilot finished work on behalf of jtramm June 26, 2025 18:33
@yrrepy
Copy link
Contributor

yrrepy commented Jun 27, 2025

Curious about the Black Arts,
did Copilot do this on its own, or did you have to ask (steer) it to do so?

@shimwell
Copy link
Member

@Copilot it looks like the formatting of the C++ files is failing the CI checks

@jon-proximafusion
Copy link
Contributor

@Copilot it looks like the formatting of the C++ files is failing the CI checks, can you correct the formatting of the C++ files you changed

Copy link
Author

Copilot AI commented Jun 27, 2025

@copilot it looks like the formatting of the C++ files is failing the CI checks

Fixed the C++ formatting issue. The multi-line string in the error message now properly conforms to the 80-character column limit as required by the project's clang-format configuration. Changes made in commit 7211442.

Copilot finished work on behalf of jtramm June 27, 2025 13:46
@jtramm
Copy link
Contributor

jtramm commented Jun 27, 2025

@yrrepy - I had opened a github issue for this item as I was planning to fix it slightly later but didn't want to forget. I noticed that when creating the issue, it allowed me to "assign" the issue to copilot instead of a human user, so figured I'd give it a try. It looks like it will only respond to comments from the person who assigned it the task.

@jtramm jtramm marked this pull request as ready for review June 27, 2025 22:42
@jtramm
Copy link
Contributor

jtramm commented Jun 27, 2025

This looks good to me. However, I think a good policy for agent generated PR's is to have the formal reviewer still be someone separate from the one who assigned the task and worked with the agent to make the PR.

@shimwell
Copy link
Member

I would be tempted to add a test that shows this fatal_error is raised appropriately.

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.

Random Ray Solver Bug When Multiple Point Sources in Same Source Region
5 participants