-
Notifications
You must be signed in to change notification settings - Fork 12
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
adding 3d mesh tallies #645
Conversation
Codecov Report
@@ Coverage Diff @@
## develop_combine #645 +/- ##
==================================================
Coverage ? 98.49%
==================================================
Files ? 68
Lines ? 4110
Branches ? 0
==================================================
Hits ? 4048
Misses ? 62
Partials ? 0 Continue to review full report at Codecov.
|
…adding_3d_mesh_tally
…adding_3d_mesh_tally
The refactoring has reduced the number of lines and therefore the coverage percent has dropped a bit. |
def mesh_tally_3D(self, value): | ||
if value is not None: | ||
if not isinstance(value, list): | ||
raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a TypeError ? :-D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah you caught me copying and pasting from the 2d mesh @Property and forgot to implement the pullrequest review suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed a few more of these in f9f7667
score = '(n,Xt)' # where X is a wild card | ||
prefix = 'tritium_production' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When standard_tally is tritium_production, these lines aren't tested
…adding_3d_mesh_tally
I am updating the dependencies image so that it contains a pip install vtk command |
I am not quite sure why the latest Circle CI tests are failing. It appears to be due to the Sweep tests. The dependency image was recently updated so perhaps that is the cause |
@shimwell are the tests failing locally ? |
…adding_3d_mesh_tally
…adding_3d_mesh_tally
Circle CI tests for the sweep are still failing, This is strange because:
|
Are you sure the image you are using for local testing is the same as the one used by the CI ? |
ah sorry when I mentioned the tests pass locally it is not on a docker image but it is on my base system. I can try locally with the docker image. Thanks for the idea |
The plot thickens as I just noticed that PR #648 is failing the same sweep tests. This is again happening with the Circle CI but not with Github Actions CI. |
Proposed changes
This PR will add the ability for users to add 3D mesh tallies to the simulation
This first commit is just a small refactoring of the existing cell and mesh tally logic
Not sure if the code from the workshop for getting mesh data into vtk files should be added or if it should run as an external script
https://github.com/ukaea/openmc_workshop/blob/1a4abd540c0bb18b5ac6115a7a127892fd776d97/tasks/task_08_CSG_mesh_tally/openmc-statepoint-3d.py#L53-L162
Types of changes
Checklist