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

Geometron #2744

Merged
merged 17 commits into from
Jan 16, 2024
Merged

Geometron #2744

merged 17 commits into from
Jan 16, 2024

Conversation

gridley
Copy link
Contributor

@gridley gridley commented Oct 22, 2023

Description

Cherry pick out geometron changes from #2655 to its own PR.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@gridley gridley requested a review from pshriwise as a code owner October 22, 2023 19:58
@gridley
Copy link
Contributor Author

gridley commented Oct 31, 2023

Hm. I thought I had removed the ParticleLostException code previously but somehow it ended up in here while cherry picking. Will have to fix that.

@jtramm
Copy link
Contributor

jtramm commented Nov 21, 2023

This will be really useful -- planning to leverage this for use with random ray. Thanks for breaking it out into its own PR. Once ParticleLostException code is removed and the merge conflict fixed, I'd be happy to review this one.

@gridley
Copy link
Contributor Author

gridley commented Dec 8, 2023

Hey John, makes sense! In the plotting PR I had removed that exception-based approach, but somehow it made its way back in while cherry picking. Silly me.

Sorry to go off the grid on this. Will hopefully be able to tie in these changes pretty soon. I've kind of wanted to try making the OpenMC geometry APIs even more generic after this, maybe removing reliance on global state a bit more so that the very robust CSG capability here can be used as a library for, say, other particle or ray-based apps. For example it would be very interesting to write some particle hydrodynamics code that uses OpenMC geometry methods.

@gridley
Copy link
Contributor Author

gridley commented Jan 4, 2024

@jtramm this should be all good to review now!

@gridley gridley mentioned this pull request Jan 9, 2024
5 tasks
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.

I flagged a few questions I had for you on the mark_as_lost() strategy, but besides that, this looks great. Thanks for putting it together!

src/geometry.cpp Show resolved Hide resolved
src/particle.cpp Show resolved Hide resolved
include/openmc/particle_data.h Show resolved Hide resolved
Comment on lines 28 to 31
void Geometron::mark_as_lost(const char* message)
{
fatal_error(message);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

At first glance, this seems problematic as the function is very different than the Particle::mark_as_lost() version, but the Geometron version is called even when tracking MC particles in some locations. Thus, it seems like it may be the case that certain types of lost particle events that previously just resulted in a warning message (not that uncommon for super large runs) would now result in a fatal error?

src/particle.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR @gridley and sorry I haven't had a chance to look at it until now! I like the direction this moves us toward (cleaner separation of geometry from physics). Other than @jtramm's comments about mark_as_lost, I only have a few small requests:

include/openmc/particle_data.h Outdated Show resolved Hide resolved
include/openmc/particle_data.h Outdated Show resolved Hide resolved
src/geometry.cpp Outdated
@@ -463,7 +465,7 @@ BoundaryInfo distance_to_boundary(Particle& p)
extern "C" int openmc_find_cell(
const double* xyz, int32_t* index, int32_t* instance)
{
Particle p;
Geometron p;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to rename the variable to better reflect the fact it is no longer a full particle (geom_state instead of p?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yes good point!

@gridley
Copy link
Contributor Author

gridley commented Jan 13, 2024

Comments should be addressed now!

@gridley
Copy link
Contributor Author

gridley commented Jan 14, 2024

Alright, CI is finally passing. I found my other PC was changing the results of a projection plot test slightly and I thought it might be due to some floating point noncommutativity that this PR was (somehow?) introduction, since the overall result looked the same. I tried just pasting in the old hash of the reference test results but that includes a newline, which the test references do not. So I went back and forth a few times on that. Phew!

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

Thanks again @gridley! 🚀

@paulromano paulromano merged commit 5549b58 into openmc-dev:develop Jan 16, 2024
18 checks passed
@gridley gridley deleted the geometron branch January 16, 2024 18:05
church89 pushed a commit to openmsr/openmc that referenced this pull request Jul 18, 2024
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