Skip to content

cy mesh origin from start of z_grid and _r_grid - #2680

Merged
pshriwise merged 3 commits into
openmc-dev:developfrom
shimwell:fixing_origin_on_cy_mesh_try2
Sep 4, 2023
Merged

pshriwise merged 3 commits into
openmc-dev:developfrom
shimwell:fixing_origin_on_cy_mesh_try2

Conversation

@shimwell

@shimwell shimwell commented Sep 4, 2023 •

Copy link
Copy Markdown
Member

Description

This PR attempts to fix the cylindrical mesh origin again 😊

PR #2676 put the origin at the center of the mesh when it should have been at the start of the mesh.

Totally my fault, hopefully this fix is actually right this time. I added more tests to check off axis behavior

Fixes # (issue)

Checklist

  • I have performed a self-review of my own code
  • I have followed the style guidelines for Python source files (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@pshriwise pshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good @shimwell! Sorry for the ask on out-of-scope lines, but I noticed that the comment on above the declaration of cached_bb is DAGMC-specific. It's beneficial to pre-compute the bounding box in either CSG or DAGMC geometry. Would you mind updating that to something more general?

@pshriwise pshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @shimwell!

@pshriwise
pshriwise merged commit a42b9ba into openmc-dev:develop Sep 4, 2023
stchaker pushed a commit to stchaker/openmc that referenced this pull request Oct 25, 2023
apingegno pushed a commit to apingegno/openmc that referenced this pull request May 7, 2026
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.

2 participants