Skip to content
This repository has been archived by the owner on Aug 15, 2023. It is now read-only.

Wrapping dagmc bounding box #95

Merged
merged 15 commits into from
Dec 8, 2021
Merged

Conversation

shimwell
Copy link
Member

@shimwell shimwell commented Dec 8, 2021

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

@shimwell shimwell changed the base branch from main to develop December 8, 2021 01:07
@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

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

❗ 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%) ⬆️
openmc_dagmc_wrapper/Materials.py 100.00% <0.00%> (ø)
openmc_dagmc_wrapper/utils.py 82.45% <0.00%> (+4.19%) ⬆️

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
Copy link
Member

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

@shimwell
Copy link
Member Author

shimwell commented Dec 8, 2021

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

@shimwell
Copy link
Member Author

shimwell commented Dec 8, 2021

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
Copy link
Member

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
Copy link
Member Author

shimwell commented Dec 8, 2021

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
Copy link
Member Author

shimwell commented Dec 8, 2021

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

@RemDelaporteMathurin
Copy link
Member

RemDelaporteMathurin commented Dec 8, 2021

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
Copy link
Member Author

shimwell commented Dec 8, 2021

Made some changes, now passing locally

@RemDelaporteMathurin
Copy link
Member

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
Copy link
Member

However this test exists in main

def test_correct_resolution(self):

@shimwell
Copy link
Member Author

shimwell commented Dec 8, 2021

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

@RemDelaporteMathurin
Copy link
Member

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
Copy link
Member Author

shimwell commented Dec 8, 2021

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
Copy link
Member Author

shimwell commented Dec 8, 2021

tests are passing, good to merge?

@RemDelaporteMathurin
Copy link
Member

Sounds good to me!

@shimwell shimwell merged commit 6880902 into develop Dec 8, 2021
@shimwell shimwell deleted the wrapping_dagmc_bounding_box branch December 8, 2021 15:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants