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

Centre for cylinder & spherical meshes #2256

Conversation

RemDelaporteMathurin
Copy link
Contributor

@RemDelaporteMathurin RemDelaporteMathurin commented Oct 7, 2022

This fixes #2248 by adding a centre attribute to CylindricalMesh and SphericalMesh

Usage

mesh = openmc.CylindricalMesh()
mesh.phi_grid = np.linspace(0, 2*np.pi, 21)
mesh.z_grid = np.linspace(-1, 1, 11)
mesh.r_grid = np.linspace(0, 1, 11)

mesh.origin = [0.1, 0.2, 0.3]

@RemDelaporteMathurin
Copy link
Contributor Author

@pshriwise I need to declare centre in mesh.cpp. However as I don't speak C++ (yet?) I don't really know how to do this. Would you mind giving this a look?

src/mesh.cpp Outdated Show resolved Hide resolved
@RemDelaporteMathurin
Copy link
Contributor Author

@pshriwise I need to declare centre in mesh.cpp. However as I don't speak C++ (yet?) I don't really know how to do this. Would you mind giving this a look?

I think I managed to add everything we needed for this in the end 😄

@RemDelaporteMathurin
Copy link
Contributor Author

The tests are failing.

One regression test is failing --> I think we just need to update the gold standard input file.
Some unit tests are failing in tests/unit_tests/test_deplete_activation.py although I'm not certain why: @shimwell do you confirm the default centre for cylindrical and spherical meshes is (0, 0, 0) ?

@RemDelaporteMathurin
Copy link
Contributor Author

I honestly don't understand why these four tests are failing.

When I run the tests with pytest tests/ they fail but when I run pytest tests/unit_tests/test_deplete_activation.py it succeeds...

@pshriwise
Copy link
Contributor

@pshriwise I need to declare centre in mesh.cpp. However as I don't speak C++ (yet?) I don't really know how to do this. Would you mind giving this a look?

I think I managed to add everything we needed for this in the end 😄

That's great! I'll take a look first thing on Monday -- I'm off today for a long weekend.

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Hi @RemDelaporteMathurin! Thanks for taking this on. 🤩 It's been needed for a while and will be a much-used, much-appreciated upgrade.

A couple of minor comments from me here. Additionally, I'd prefer to use the term origin over centre here to be consistent with our description of a sphere surface. Would you mind updating that attribute name?

include/openmc/mesh.h Outdated Show resolved Hide resolved
src/mesh.cpp Outdated Show resolved Hide resolved
openmc/mesh.py Outdated Show resolved Hide resolved
Co-authored-by: Ethan Peterson <[email protected]>
openmc/mesh.py Outdated Show resolved Hide resolved
Copy link
Contributor

@eepeterson eepeterson left a comment

Choose a reason for hiding this comment

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

@RemDelaporteMathurin and @pshriwise just a few more questions and comments

docs/source/io_formats/statepoint.rst Outdated Show resolved Hide resolved
include/openmc/mesh.h Outdated Show resolved Hide resolved
tests/unit_tests/test_cylindrical_mesh.py Outdated Show resolved Hide resolved
tests/unit_tests/test_spherical_mesh.py Outdated Show resolved Hide resolved
@pshriwise
Copy link
Contributor

I've just attempted to handle the conflicts in mesh.py. I'll keep an eye on CI to make sure that was successful.

Aside from that, I think there's one outstanding comment from @eepeterson remaining. @RemDelaporteMathurin are you up for addressing that one?

@RemDelaporteMathurin
Copy link
Contributor Author

RemDelaporteMathurin commented Mar 16, 2023

@RemDelaporteMathurin are you up for addressing that one?

Thanks for this Patrick, I don't have the bandwidth right now but I can have a look whenever I have time

@paulromano
Copy link
Contributor

@RemDelaporteMathurin I just sent a PR to your fork to address the outstanding request from @eepeterson:
RemDelaporteMathurin#7

@RemDelaporteMathurin
Copy link
Contributor Author

@paulromano @eepeterson @pshriwise I think all the comments have been addressed and the tests pass! 🎸

Copy link
Contributor

@eepeterson eepeterson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the work team!

@eepeterson eepeterson merged commit 572e765 into openmc-dev:develop Mar 23, 2023
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.

Cylinder meshes to allow non central location
5 participants