Skip to content

Addition of a collision tracking feature #3417

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 14 commits into
base: develop
Choose a base branch
from

Conversation

saliba170499
Copy link

Greetings,

I added a collision tracking feature. the user can select parameters to select/filter for specific collision types. The information is saved in a collision_track.h5 (or .mcpl) file. The data can then be extracted via the python api.

The formatting of the C++ code was done using clang-format15. Python follows the PEP8 format. Documentation has been added. Unit and regression tests were added.

Looking forward to an eventual merging in the dev branch!

Thank you very much for your time for reviewing and your support,

Kind regards,
Michel Saliba
PhD at LRS - EPFL

@saliba170499 saliba170499 requested a review from ebknudsen as a code owner May 23, 2025 13:43
@MicahGale
Copy link
Contributor

Is this related to this paper:

Development of an Event Tracking Feature in OpenMC for Neutron Spectroscopy and Scatter Camera Systems

10:50–11:15AM MDT

Michel Saliba (École Polytechnique Fédérale de Lausanne), Paul Romano (ANL), John Tramm (ANL), Erik B. Knudsen (United Neux), Daniel Siefman (Univ. California, Berkeley), Andreas Pautz (Paul Scherrer Institute), Oskari Pakari (École Polytechnique Fédérale de Lausanne)

@jtramm
Copy link
Contributor

jtramm commented Jun 10, 2025

@MicahGale - yes, this is the feature from that paper.

@jtramm
Copy link
Contributor

jtramm commented Jun 10, 2025

Thanks for submitting this @saliba170499! Generally the PR is looking good -- code looking clean and should make for a useful feature!!

A few high level topics before I go through and make finer-grained comments:

  1. It looks like the C++ formatting checker is failing. I'm not sure what would be causing that failure, as you mentioned you had run v15 of clang-format already, but perhaps it is not finding the config file somehow. Try running it again on your end (or on another machine) to see if it makes more changes. For reference, the CI gives the following complaints:
  Notice: File include/openmc/mcpl_interface.h does not conform to Custom style guidelines. (lines 34, 43)
  Notice: File include/openmc/settings.h does not conform to Custom style guidelines. (lines 159, 160, 161)
  Notice: File include/openmc/state_point.h does not conform to Custom style guidelines. (lines 46, 53)
  Notice: File src/settings.cpp does not conform to Custom style guidelines. (lines 974)
  1. I'm noticing that some of the results_true.dat files just have k eigenvalue information in them, which doesn't seem useful for testing this feature, though perhaps the event files are tested using another strategy? It would be helpful if you could give some high level details on your testing strategy as part of the PR writeup (or a comment on the discussion here). Also, what is the purpose of all the different test cases (e.g., case-01, case-02, ..., case-a02?) If they are testing different types of conditions, might be useful to give them each a more descriptive name. For long term maintenance, if one breaks, it is nice to know right away what is broken based on the test name.

Not a big deal, but for future PRs, it is preferable to restrict auto formatter changes to only lines of code that you worked on. C++ files are usually safe to do globally as the clang-format program is very consistent, but the style guidelines for python aren't as well defined. Running it globally on python scripts generates a much larger diff which makes reviewing more time consuming/painful.

Anyway, thanks again for putting this in! I'll make a finer-grained pass once the tests are passing.

@saliba170499
Copy link
Author

Thank you very much for your review, @jtramm.

This all makes sense. I will address your points and provide a short outline of what the tests are probing for.

@saliba170499
Copy link
Author

saliba170499 commented Jun 23, 2025

@jtramm ,I've been thinking about the testing strategy, and I agree the current structure could be clearer. I'm thinking of re-designing the tests like this:

Tests directly tied to user input parameters:

  • case_1_MT_number: Tests tracking collisions with only MT numbers set, like:
    'settings.collision_track = { 'max_collisions': 300, 'mt_numbers': [2,18] }'
  • case_2_Cell_ID: Uses only the cell ID filter.
  • case_3_Material_ID: Checks material ID filtering alone.
  • case_4_Nuclide_ID: Tests with nuclide IDs as the only discriminator.
  • case_5_Universe_ID: Universe IDs only, to confirm this filter individually.
  • case_6_deposited_energy_threshold: Collision tracking with a deposited-energy threshold specified, verifying threshold logic.
  • case_7_all_parameters_used_together: A comprehensive “kitchen sink” test using every available input parameter simultaneously.

I agree that just looking at k-eff mean and standard deviation in results_true.dat doesn't strongly validate this particular feature. However, the main correctness check will always be comparing generated collision-track HDF5 files (collision_track.h5) against reference outputs (collision_track_true.h5). Those comparisons would be hopefully enough to catch any bugs introduced later.

Additional tests (already included in the current PR):

  • A test with 2 OpenMP threads, to confirm thread safety. case-a01 will be renamed to case_8_2threads
  • An event-based mode test, this is case-e01 to be renamed case_9_event_mode, since the above cases primarily use history-based simulations. I'll consider adding another event-based test with 2 threads, to fully ensure thread safety in the event-based solver too.

Let me know if this makes sense or if you have any other suggestions! Upon your confirmation on the regression test strategy, I will make the changes and add them to this PR asap.

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