fusion-energy / openmc-dagmc-wrapper

A Python package that extends OpenMC base classes to provide convenience features and standardized tallies when simulating DAGMC geometry with OpenMC.
https://openmc-dagmc-wrapper.readthedocs.io/
MIT License
7 stars 2 forks source link

Wrapping dagmc bounding box #95

Closed shimwell closed 2 years ago

shimwell commented 2 years ago

This adds a corners() method to the Geometry class.

corners can be used to find the bounding box coordinates which is often needed for 2d or 3d mesh tallies

This makes use of the dagmc-bounding-box package

Tests have been added to check the corners are accessible and correct.

Examples have been updated

codecov[bot] commented 2 years ago

Codecov Report

Merging #95 (f49d0ea) into develop (3d0f7b8) will increase coverage by 0.60%. The diff coverage is 100.00%.

:exclamation: Current head f49d0ea differs from pull request most recent head 77958d8. Consider uploading reports for the commit 77958d8 to get more accurate results Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #95      +/-   ##
===========================================
+ Coverage    87.74%   88.34%   +0.60%     
===========================================
  Files            6        6              
  Lines          310      326      +16     
===========================================
+ Hits           272      288      +16     
  Misses          38       38              
Impacted Files Coverage Δ
openmc_dagmc_wrapper/Geometry.py 80.00% <100.00%> (+2.22%) :arrow_up:
openmc_dagmc_wrapper/Materials.py 100.00% <0.00%> (ø)
openmc_dagmc_wrapper/utils.py 82.45% <0.00%> (+4.19%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3d0f7b8...77958d8. Read the comment docs.

RemDelaporteMathurin commented 2 years ago

That's really convenient @shimwell . Can the MeshTally tests be modified accordingly then?

shimwell commented 2 years ago

I can do that just to simplify things, but I don't think that wouldn't test anything new.

shimwell commented 2 years ago

So this helps simplify the user facing workflow a little bit as one less tool is needed by the users.

User facing tools is just a subsection of all the tools available and are in blue on this diagram

neutronics_workflow drawio(5) .

RemDelaporteMathurin commented 2 years ago

I can do that just to simplify things, but I don't think that wouldn't test anything new.

I think it would cause, as far as I know, the examples aren't tested in GA or CircleCI. Which means that the integration of this new feature with the Mesh tallies isn't tested.

shimwell commented 2 years ago

I've updated the meshtally tests to make use of this and also updated the test in tests/test_system/test_system_single_volume.py to make use of this new feature

shimwell commented 2 years ago

codiga comments are mainly regarding the url download for testing, this is a known problem

RemDelaporteMathurin commented 2 years ago

I've updated the meshtally tests to make use of this and also updated the test in tests/test_system/test_system_single_volume.py to make use of this new feature

These tests now fail. Must be due to a missing import https://app.circleci.com/pipelines/github/fusion-energy/openmc-dagmc-wrapper/451/workflows/515946cc-69ee-4e24-b0ed-1ceaf9d047c1/jobs/447

shimwell commented 2 years ago

Made some changes, now passing locally

RemDelaporteMathurin commented 2 years ago

Hm there's is something really weird here.

The worflow "CI with install" is failing because of the test FAILED tests/test_tallies/test_mesh_tally_2d.py::TestMeshTally2D::test_correct_resolution

This is the error message:

=================================== FAILURES ===================================
___________________ TestMeshTally2D.test_correct_resolution ____________________

self = <tests.test_tallies.test_mesh_tally_2d.TestMeshTally2D testMethod=test_correct_resolution>

    def test_correct_resolution(self):
        """Tests that the mesh resolution is in accordance with the plane
        """
        tally_xy = odw.MeshTally2D(
            tally_type="neutron_flux",
            plane="xy",
>           bounding_box=DagmcBoundingBox(self.h5m_filename_smaller).corners(),
            mesh_resolution=(10, 20),
        )
E       NameError: name 'DagmcBoundingBox' is not defined

tests/test_tallies/test_mesh_tally_2d.py:100: NameError

Now what's funny is that if we look at CircleCI, this test doesn't exist image

In fact, the supposedly failing line 100 doesn't even exist in the file tests/test_tallies/test_mesh_tally_2d.py ....

RemDelaporteMathurin commented 2 years ago

However this test exists in main https://github.com/fusion-energy/openmc-dagmc-wrapper/blob/3d0f7b8aa1448195d313d345bb0283f508e56368/tests/test_tallies/test_mesh_tally_2d.py#L95

shimwell commented 2 years ago

ah was I out of sync with develop, thanks for that merge

RemDelaporteMathurin commented 2 years ago

I re-synced the branch with develop with the last commit. The problem came from the fact that #93 wasn't merged in develop but in main that was my mistake sorry about that

shimwell commented 2 years ago

I think the docker images building can also be improved. I think this can be fixed by adding docker layer caching to the build docker images script. But perhaps that should be solved that in another PR.

shimwell commented 2 years ago

tests are passing, good to merge?

RemDelaporteMathurin commented 2 years ago

Sounds good to me!