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

Monte carlo packets #4089

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Conversation

ffoucart
Copy link
Contributor

@ffoucart ffoucart commented Jul 1, 2022

Proposed changes

Add packets for MC evolutions, and ability to evolve packets along a geodesic

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

Very preliminary; packets cannot at this point be included in actual evolution...

@ffoucart ffoucart added the in progress Don't review, used for sharing code and getting feedback label Jul 1, 2022
@ffoucart ffoucart force-pushed the MonteCarloPackets branch 3 times, most recently from ae1cf13 to 183cff1 Compare February 27, 2023 16:31
${LIBRARY}
"Evolution/Particles/MonteCarlo"
"${LIBRARY_SOURCES}"
"Boost::boost;Evolution;Utilities"
Copy link
Member

Choose a reason for hiding this comment

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

you are missing MonteCarlo in this list

Better is to update this to the following (which is what we are slowly doing to all test directories):

  • Change line 14 to just be an empty list ""
  • Add the following to the end of the file:
target_link_libraries(
  ${LIBRARY}
  PRIVATE
  Monte Carlo
  )

@ffoucart ffoucart force-pushed the MonteCarloPackets branch 2 times, most recently from 592960a to aac9a69 Compare February 28, 2023 00:58
@ffoucart ffoucart force-pushed the MonteCarloPackets branch 2 times, most recently from d18e135 to fe0c3d9 Compare March 15, 2023 15:47
@ffoucart
Copy link
Contributor Author

@kidder : The code for the evolution of a single packet for a time step along a geodesic seems to pass the standard tests now. I'm not sure what a more appropriate test would be here; the one I implemented is obviously simplistic. I could write a python function reproducing the whole second-order time stepping, and compare results with random inputs, I guess? It's not clear to me whether we will actually perform time steps using this method, or incorporate the time-stepping within another part of the code once we try to actually move packets on a grid.

Apart from that, I think this PR is ready for review (and, obviously, fairly low priority...).

@ffoucart ffoucart removed the in progress Don't review, used for sharing code and getting feedback label Mar 15, 2023
Copy link
Member

@nilsdeppe nilsdeppe 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! A few small comments you can squash right away when you rebase

@ffoucart ffoucart force-pushed the MonteCarloPackets branch 6 times, most recently from 41494bb to 4f7fe63 Compare February 15, 2024 15:34
@ffoucart
Copy link
Contributor Author

@nilsdeppe More changes than I'd have liked in order to match the latest clang-tidy requirements, but this should be ready for you again. Note that I needed to add a default constructor to the packet struct -- clang-tidy was unhappy with packet creation that did not explicitly set the contents of the packet.

@ffoucart
Copy link
Contributor Author

(The failing test is some paraview thing... I'm guessing this has nothing to do with this PR)

@ffoucart ffoucart mentioned this pull request Feb 15, 2024
3 tasks
@nilsdeppe
Copy link
Member

CI failure is some unrelated paraview thing.

@nilsdeppe nilsdeppe merged commit 09a9173 into sxs-collaboration:develop Feb 19, 2024
21 of 22 checks passed
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