fix - 2939 #3007

caderache2014 · 2024-05-15T22:57:57Z

Adding basic contains functionality for cell in Geometry and Universe.

Fixes # (2939)

Checklist

[x]x] I have performed a self-review of my own code
I have run clang-format (version 15) on any C++ source files (if applicable)
I have followed the style guidelines for Python source files (if applicable)
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works (if applicable)

MicahGale · 2024-05-21T19:34:52Z

openmc/universe.py

+        if self.cells is None:
+            return False
+        else:
+            return cell.id in self.cells.keys()


Can just be cell.id in self.cells

MicahGale · 2024-05-21T19:36:04Z

To link it to the issue just say : fixes #2939

caderache2014 · 2024-05-22T01:01:29Z

@MicahGale @paulromano @pshriwise Hi all... looks like we have a failure.

It looks the like unit_tests/test_tracks.py has failed which is outside the remit this fix. Thoughts all?

paulromano

The changes here don't really match what was proposed in #2939 since they only check for containment against cells in the immediate universe (rather than all cells that are potentially contained within a universe, which would require calling get_all_cells())

caderache2014 · 2024-05-27T00:07:40Z

@paulromano @MicahGale Please take a look at this revised PR. Am I on the right track?

MicahGale

Overall this is a good second attempt. I think it could just be made more efficient.

MicahGale · 2024-05-28T15:10:03Z

openmc/geometry.py

+        if cell is None:
+            return False


I don't think this should be valid. It should raise a TypeError. This could be:

if not isinstance(cell, openmc.Cell): raise TypeError(...)

but I think there's check_value, which is the preferred way to do type enforcement.

MicahGale · 2024-05-28T15:10:45Z

openmc/geometry.py

+        if cell in set(self.get_all_cells().values()):
+            return True 
+
+        if cell in set(self.get_all_universes().values()):
+            return True  
+
+        return False


This should all be punted to the Universe implementation.

MicahGale · 2024-05-28T15:10:59Z

openmc/geometry.py

+        if cell in self.root_universe:
+            return True  


Just do return cell in self.root_universe.

MicahGale · 2024-05-28T15:11:15Z

openmc/universe.py

+        if cell is None:
+            return False


Once again check_value?

MicahGale · 2024-05-28T15:17:54Z

openmc/universe.py

+        local_cells = self.get_all_cells().values()
+
+        if local_cells is None:
+            return False
+
+
+
+        if cell in local_cells:
+            return True
+
+
+        for univ in self.get_all_universes().values():
+            if cell in univ:
+                return True
+
+        return False


I don't like this because it requires the entire tree to be flattened prior any searching being performed, which is very time consuming (#2953). This should be a short-circuited depth-first search (sorry C.S. people for butchering that).

I think it should be something like:

def __contains__(self, cell): ... type stuff # succeed in constant time if the cell is a leaf if cell.id in self.cells: return True # otherwise start a depth-first search for child_cell in self.cells.values(): if isinstance(child_cell.fill, openmc.Universe): if cell in child_cell.fill: return True return False

MicahGale · 2024-05-28T15:23:07Z

tests/unit_tests/test_geometry.py

+def test_contains_cell():
+
+    c1 = openmc.Cell()
+    c2 = openmc.Cell()
+    c3 = openmc.Cell()
+    c3 = openmc.Cell()
+    c4 = openmc.Cell()
+
+
+    geom1= openmc.Geometry([c1, c2, c3])
+
+    assert (c1 in geom1)
+    assert (c2 in geom1)
+    assert (c3 in geom1)
+    assert not (c4 in geom1)
+
+
+    univ1 = openmc.Universe(name='univ1', cells=[c1, c2, c3])
+    geom2 = openmc.Geometry(univ1)
+
+    assert (c1 in geom2) 
+    assert (c2 in geom2)
+    assert (c3 in geom2)
+    assert not (c4 in geom2)
+
+
+    c5 = openmc.Cell()
+    c6 = openmc.Cell()
+    univ2 = openmc.Universe(name='univ2', cells=[c5, c6])
+    c7 = openmc.Cell(fill=univ2)
+    c8 = openmc.Cell(fill=univ1)
+    univ_comp = openmc.Universe(name='composed universe',cells=[c7, c8])
+    geom_comp = openmc.Geometry(univ_comp)
+
+    assert c5 in geom_comp
+    assert c6 in geom_comp
+    assert c7 in geom_comp
+    assert c8 in geom_comp
+    assert c1 in geom_comp
+
+
+


A few thoughts:

I don't think this is quite PEP8, but I don't have the style off the top of my head.

Line 27 could just be geom1.root_universe = univ1 but isn't essential.

The test suite looks really good for having wide coverage of all possible scenarios.

There is some repeated code, I can't decide if the effort of making it DRY is worth it here though. It is a test suite after all.

Also you should test that nonsense fails, like testing that an error is raised for "hi" in geometry.

caderache2014 · 2024-05-31T02:03:47Z

@MicahGale @paulromano next draft submitted. All tests have passed. Any thoughts?

MicahGale

This is getting really close, I just think TypeError (or whatever exception check_type raises shouldn't be suppressed.

I think this is mature enough for @paulromano, @pshriwise, or another core dev to review.

MicahGale · 2024-06-04T15:21:09Z

openmc/geometry.py

+        try:
+            check_type('cell', cell, openmc.Cell)
+        except:
+            return False


I don't think it's appropriate to silently suppress a TypeError. This shouldn't be in a try-except block IMO.

MicahGale · 2024-06-04T15:22:20Z

openmc/universe.py

+        if cell.id in self.cells:
+            return True
+
+        #otherwise do a depth-first search
+        for child_cell in self.cells.values():
+            if isinstance(child_cell.fill, openmc.Universe):
+                if cell in child_cell.fill:
+                    return True
+        return False


This code looks great... and familiar 😆.

MicahGale · 2024-06-04T15:24:00Z

tests/unit_tests/test_universe.py

+    assert c9 in u_composed
+    assert not c4 in u_composed 
+
+    assert not 'hi' in u_composed


I think this should be:

with pytest.raises(TypeError): #error type might be different I'm just assuming 'hi' in u_composed

@MicahGale I'm not sure I understand. I altered the lines of test_universe.py to:

This indeed triggers the TypeError exception from the logic of universe.py to

This produced the following lone error when I run pytest test_universe.py:

Is there example of raising TypeError within the in another test script that I could look at? I assume I'm missing some syntax to.... contain the TypeError exception within the assert statement (analougous to a try-catch block)

@MicahGale never mind. I figured it out

MicahGale

As I said I think you added some tests that aren't relevant here and should be their own PR. They seem to be in commit: a88be0f.

To move them to a new branch you should:

Make a new branch from develop: git checkout develop && git checkout -b <foo>. Though you are making this PR from your develop branch so tracking so making a new branch from the upstream develop will be tricky. You can make a new branch from your develop the more I think about it. It's not best practice as the PR for the other branch will include changes from this PR, but then you will just have to make sure the PRs are merged in the correct order.
Cherry pick the commit to the new branch: git cherry-pick a88be0f602f897abe7406f83a9bf1709587d68cb
Revert the commit on this branch. git checkout develop && git revert a88be0f602f897abe7406f83a9bf1709587d68cb

You will see people online saying you should do a hard reset, in general they are wrong and git reset should only be used if you really know what you're doing.

MicahGale · 2024-06-05T13:44:31Z

tests/unit_tests/test_lib.py

+
+def test_boundingbox_consistency():
+
+    s19 = openmc.YCylinder(0, 0, 17.7)
+    s48 = openmc.Plane(0.7071067811865476, 6.123233995736766e-17, 0.7071067811865476, 11.45)
+    s58 = openmc.Plane(0.7071067811865476, 6.123233995736766e-17, 0.7071067811865476, 14.35)
+    s62 = openmc.Plane(6.123233995736766e-17, 1.0, 6.123233995736766e-17, 1.5999999999999999)
+    s64 = openmc.Plane(6.123233995736766e-17, 1.0, 6.123233995736766e-17, -1.3)
+    s65 = openmc.Plane(-0.7071067811865475, 6.123233995736766e-17, 0.7071067811865476, 1.45)
+    s68 = openmc.Plane(-0.7071067811865475, 6.123233995736766e-17, 0.7071067811865476, -1.45)
+    s101 = openmc.YPlane(5.6, boundary_type='vacuum')
+
+    region = +s64 & -s101 & -s19 & +s48 & (+s58 | +s62 | -s64 | +s65 | -s68)
+    cell = openmc.Cell(region=region)
+
+    univ = openmc.Universe(cells=[cell])
+    geometry = openmc.Geometry(root=univ)
+    settings = openmc.Settings(particles=100, batches=100)
+    model = openmc.model.Model(geometry=geometry, settings=settings)
+    model.export_to_model_xml()
+
+    openmc.lib.init()
+    #print(cell.bounding_box)
+    py_lowerleft, py_upperright = cell.bounding_box
+    assert tuple(py_lowerleft) == (-17.7, -1.3, -17.7)
+    assert tuple(py_upperright) == (17.7, 5.6, 17.7)
+    #print(openmc.lib.cells[1].bounding_box)
+
+    cpp_lowerleft, cpp_upperright = openmc.lib.cells[1].bounding_box
+    assert tuple(cpp_lowerleft) == (-17.7, -inf, -17.7)
+    assert tuple(cpp_upperright) == (17.7, 5.6, 17.7)
+
+    openmc.lib.finalize()


How is this topical to this PR?

What value is this adding beyond those already in tests/unit_tests/test_bounding_box.py?

Generally it's bad practice to have something that must always run to free resources (i.e., finalize) outside of a try... finally. with openmc.lib.run_in_memory(): is an option.

Don't use == with a floating point for testing.

If you moved this to a separate PR I'd be ready to approve.

@MicahGale sorry this is not relevant to this PR. This is one single part of a separate bug I'm working on. I do have a separate local branch called fix_2939 but.... somehow something got corrupted into my develop branch. I'll rebase and figure it out. Again, apologies

caderache2014 · 2024-06-05T19:30:20Z

@MicahGale approved? CI testing isn't done yet..... @paulromano @pshriwise thoughts?

caderache2014 · 2024-06-08T21:23:16Z

@paulromano did you have any notes? I think @MicahGale is satisfied...?

caderache2014 added 3 commits May 15, 2024 14:40

Added __contains__ to geometry.py universe.py with concomitant testing

f403fdc

Syntax fix to root_universe.py

16be76c

Tweaked name of test universe

22d8c0e

caderache2014 mentioned this pull request May 16, 2024

Re: Issue #2939 - Implement contains for universe and geomery #3004

Closed

5 tasks

MicahGale reviewed May 21, 2024

View reviewed changes

caderache2014 added 2 commits May 21, 2024 16:26

tweaks to geometry.py and universe.py contains

86017f8

Merge branch 'fix_2939' into develop

52553f1

paulromano requested changes May 24, 2024

View reviewed changes

caderache2014 added 3 commits May 26, 2024 13:02

Added support for neighboring universes in fix 2939

582341e

Merge branch 'fix_2939' into develop

dd0dda7

test_gemetry and test_universe

107cae4

MicahGale suggested changes May 28, 2024

View reviewed changes

caderache2014 added 4 commits May 30, 2024 17:35

revised implementation of __contains__ for Geometry and Universe

ddff6dc

Merge branch 'fix_2939' into develop

afd93e6

added test_bounding_box_consistency() to test_lib.py and test_surface.py

a88be0f

Merge branch 'develop_backup' into develop

882097a

MicahGale suggested changes Jun 4, 2024

View reviewed changes

caderache2014 added 2 commits June 4, 2024 19:25

added raise(TypeError) to universe and geometry contains

4432e74

Merge branch 'fix_2939' into develop

ff6e5c6

MicahGale reviewed Jun 5, 2024

View reviewed changes

caderache2014 added 2 commits June 5, 2024 09:30

added test_bounding_box_consistency() to test_lib.py and test_surface.py

164975b

cherry pick commit

a4404d8

MicahGale approved these changes Jun 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix - 2939 #3007

fix - 2939 #3007

caderache2014 commented May 15, 2024

MicahGale May 21, 2024

MicahGale commented May 21, 2024

caderache2014 commented May 22, 2024

paulromano left a comment

caderache2014 commented May 27, 2024

MicahGale left a comment

MicahGale May 28, 2024

MicahGale May 28, 2024

MicahGale May 28, 2024

MicahGale May 28, 2024

MicahGale May 28, 2024

MicahGale May 28, 2024

MicahGale May 28, 2024

caderache2014 commented May 31, 2024

MicahGale left a comment

MicahGale Jun 4, 2024

MicahGale Jun 4, 2024

MicahGale Jun 4, 2024

caderache2014 Jun 5, 2024

caderache2014 Jun 5, 2024

MicahGale left a comment

MicahGale Jun 5, 2024

caderache2014 Jun 5, 2024

caderache2014 commented Jun 5, 2024

caderache2014 commented Jun 8, 2024

fix - 2939 #3007

Are you sure you want to change the base?

fix - 2939 #3007

Conversation

caderache2014 commented May 15, 2024

Checklist

Choose a reason for hiding this comment

MicahGale commented May 21, 2024

caderache2014 commented May 22, 2024

paulromano left a comment

Choose a reason for hiding this comment

caderache2014 commented May 27, 2024

MicahGale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caderache2014 commented May 31, 2024

MicahGale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicahGale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caderache2014 commented Jun 5, 2024

caderache2014 commented Jun 8, 2024