Add new refine surface, bounding box, and point refine methods. #297

jcapriot · 2022-11-23T00:22:22Z

This is intended to replace the functionality of the refine_tree_xyz utility, which will be marked as deprecated.
The new functions have slightly more intuitive inputs (in my opinion). They use the level argument to explicitly refer to the octree level, and padding_cells_by_level to refer to the amount of padding around the object.

The previous function split the padding descriptions into the octree_levels argument and the octree_levels_padding. The method of refining at specific levels is not intuitive and it was implied only by the length of the array passed to octree_levels, and not there values, as the name might imply.

~~There is still missing tests~~, but just wanted to get these out there for feedback on the inputs and descriptions in the documentation of these functions. The new surface function is not necessarily any quicker than the previous, but it is more well behaved. Although it's cost rises faster with the number of triangles in the triangulation.

codecov · 2022-11-23T01:07:07Z

Codecov Report

Merging #297 (b3d059c) into main (1788cb0) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #297      +/-   ##
==========================================
+ Coverage   85.04%   85.23%   +0.19%     
==========================================
  Files          36       36              
  Lines       11090    11239     +149     
==========================================
+ Hits         9431     9580     +149     
  Misses       1659     1659

Impacted Files	Coverage Δ
discretize/_extensions/tree_ext.pyx	`84.44% <100.00%> (+0.21%)`	⬆️
discretize/tree_mesh.py	`70.71% <100.00%> (+9.51%)`	⬆️
discretize/utils/mesh_utils.py	`87.46% <100.00%> (+0.11%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jcapriot · 2022-11-24T21:13:43Z

I'm thinking it might be a faster option to refine the triangulated surface (and not pass it as the simplexes), and shift it up and down for each padding level. The triangle refine is quicker than the tetrahedron refine as there are half as many edges to check and a third of the faces, not to mention for each triangle of the surface, it takes 3 tetrahedron to represent the triangular prism column that is currently represented.

Turns out it is about the same

domfournier · 2022-11-27T17:10:08Z

I'm thinking it might be a faster option to refine the triangulated surface (and not pass it as the simplexes), and shift it up and down for each padding level. The triangle refine is quicker than the tetrahedron refine as there are half as many edges to check and a third of the faces, not to mention for each triangle of the surface, it takes 3 tetrahedron to represent the triangular prism column that is currently represented.

Turns out it is about the same

Takes about x2.5 longer to run: for 200k triangles and ~500k cells mesh.

domfournier · 2022-11-27T17:01:09Z

discretize/utils/mesh_utils.py

+            "use the `TreeMesh.refine_points` functionality. It will be removed in a "
+            "future version of discretize.",
+            DeprecationWarning,
+        )


Strangely the warning doesn't print on my end... even though I am sure I am getting there

domfournier · 2022-11-27T17:02:18Z

discretize/_extensions/tree_ext.pyx

@@ -521,9 +521,9 @@ cdef class _TreeMesh:
        ----------


In case you are not seeing on your side - I am getting a lot of warnings when compiling the code

domfournier · 2022-11-27T17:14:36Z

discretize/tree_mesh.py

+        >>> mesh = discretize.TreeMesh([32, 32])
+        >>> points = np.random.rand(20, 2) * 0.25 + 3/8
+
+        Now we want to refine to the maximum level, with a no padding the in `x`


Suggested change

Now we want to refine to the maximum level, with a no padding the in `x`

Now we want to refine to the maximum level, with a no padding in the `x`

discretize/tree_mesh.py

domfournier · 2022-11-27T17:23:20Z

discretize/tree_mesh.py

+        finalize : bool, optional
+            Whether to finalize the mesh after the call.
+        diagonal_balance : None or bool, optional
+            Whether to balance cells diagonally in the refinement, `None` implies using


Suggested change

Whether to balance cells diagonally in the refinement, `None` implies using

Whether to balance cells diagonally in the refinement. `None` implies using

discretize/tree_mesh.py

domfournier · 2022-11-27T17:40:29Z

discretize/utils/mesh_utils.py

+        # padding[:, -1] = np.maximum(octree_levels - 1, 0)
+        # padding[:, :-1] = octree_levels_padding[:, None]
+        # padding = padding[non_zeros]
+        # mesh.refine_bounding_box(xyz, levels, padding, finalize=finalize)


Same here...

domfournier · 2022-11-27T17:40:38Z

discretize/utils/mesh_utils.py

+        # padding[:, -1] = np.maximum(octree_levels - 1, 0)
+        # padding[:, :-1] = octree_levels_padding[:, None]
+        # padding = padding[non_zeros]
+        # mesh.refine_surface(xyz, levels, padding, finalize=finalize, pad_down=True, pad_up=False)


Why not re-routing to the new method now? These lines don't run as is BTW

These are here to mostly just remind ourselves of the translations between the two methods, as their calling conventions are slightly different, and the surface refine behaves slightly differently around the edges.

discretize/tree_mesh.py

domfournier · 2022-11-27T17:59:00Z

@jcapriot Went through it with some testing. Two sticky things for me:
1- The definition of levels counting backward and padding cells seems odd an not easy to follow
2- Surface refinement should require arguments for the vertices AND simplices. It is also slower than the old method, although the 3D generalization is nice.

jcapriot · 2022-11-27T19:27:47Z

1.) In my opinion the current implementation on refine_xyz of inferring the level by the length of an input array is not intuitive. This method allows you to explicitly control which level you want to refine to. Regarding the negative input values, -1 and mesh.max_level both refer to the same thing. It's used the same way you can index the last value of an array in python with negative levels. All of the mesh.refine_ functions on tree mesh work this way.

Maybe the levels input to these functions would make more sense as a dictionary whose keys are the level, and the value is the padding? (i.e. {-1:[0, 0, 2], -2:[0, 0, 3]})
2.) Can you give me those input meshes so I can play around with the internals a bit?

domfournier · 2022-11-27T20:35:12Z

I like the dictionary idea. I still think that working with negative indexing is cumbersome as users never care for index 0, always the max level then backward. This is just internal mechanics, we could make an effort to make it more intuitive.

dccowan · 2022-11-27T21:55:24Z

Comment on efficacy for end users: I feel like most people will want to set the finest desired level of discretization, then set the number of padding cells at level as things are more coarse. So having the level argument be int would be the easiest implementation. In addition, I think new users will naturally be inclined to set the finest discretization by having level= 1 instead of level = self.max_level.

Comment on level as an array_like: If padding_cells_per_level is array_like, the function already knows what values correspond to which level even if level is int. Can someone provide a commonly encountered situation where level must be array_like in order to make the mesh they want?

jcapriot · 2022-11-28T00:07:29Z

Comment on efficacy for end users: I feel like most people will want to set the finest desired level of discretization, then set the number of padding cells at level as things are more coarse. So having the level argument be int would be the easiest implementation. In addition, I think new users will naturally be inclined to set the finest discretization by having level= 1 instead of level = self.max_level.

I'm pretty hard against referring to level as opposite to how it is used everywhere else in the codebase just for these functions. Reversing the input by starting the finest cell level as 0/1 would lead to more confusion in my opinion, especially if you look at the mesh afterwards and see 100,000 cells at the level 6. The negative indexing is meant to make it easier to refer to the finest level (instead of writing mesh.max_level or mesh.max_level - 1).

I like the dictionary idea. I still think that working with negative indexing is cumbersome as users never care for index 0, always the max level then backward. This is just internal mechanics, we could make an effort to make it more intuitive.

What would make it more intuitive? I do want to force the user to be explicit about which level they want to refine to. (I've seen use cases before where users wanted to use the refine_tree_xyz function to not refine to the max level and were very confused about how to make it do that.)

domfournier · 2022-11-28T02:45:07Z

I think if a user creates a mesh and doesn't want to refine to its lowest, then they should use a bigger core cell size. But whatever, you have the final say.

jcapriot · 2022-11-28T03:49:59Z

The example I recalled was they wanted to refine near the surface at the finest level, but then refine at a coarser level within a box.

domfournier · 2022-11-28T05:41:39Z

That's fine, most users won't interact at that level anyway. Tree mesh creation is tough already, we can just create simple UIs and do the translation under the hood.

jcapriot · 2022-11-28T17:40:39Z

What if level was a keyword argument with a default of max_level? I feel like that would be a good compromise, then also allow the padding to function like Devin’s description.

domfournier · 2022-11-28T17:49:32Z

Yea that sounds good. So the padding_cells_by_level: list[list | int] would be relative to the level: int defaulted to max_level. If a user wants to start coarser, than they need to supply the true octree level index (or -). Makes sense.

Add new refine surface, bounding box, and point refine methods.

71eb2f8

jcapriot requested a review from domfournier November 23, 2022 00:22

jcapriot added 2 commits November 22, 2022 16:35

remove refine_tree_xyz in the examples.

d2a4fd9

fix deprecation warning

8f8a8e5

jcapriot added 4 commits November 23, 2022 00:00

fix syntax

64a699c

fix working error test

c7e5bfa

add tests for new functions

7f3a956

modify test to hit last line of code

597a37f

domfournier reviewed Nov 27, 2022

View reviewed changes

jcapriot added 2 commits November 30, 2022 20:21

refine updates

4414192

documentation updates

b3d059c

jcapriot merged commit fe6931e into simpeg:main Dec 2, 2022

jcapriot deleted the refine_xyz_refactor branch December 2, 2022 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new refine surface, bounding box, and point refine methods. #297

Add new refine surface, bounding box, and point refine methods. #297

jcapriot commented Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

jcapriot commented Nov 24, 2022 •

edited

Loading

domfournier commented Nov 27, 2022 •

edited

Loading

domfournier Nov 27, 2022

domfournier Nov 27, 2022

domfournier Nov 27, 2022

domfournier Nov 27, 2022

domfournier Nov 27, 2022

domfournier Nov 27, 2022

jcapriot Nov 27, 2022

domfournier commented Nov 27, 2022

jcapriot commented Nov 27, 2022 •

edited

Loading

domfournier commented Nov 27, 2022

dccowan commented Nov 27, 2022

jcapriot commented Nov 28, 2022 •

edited

Loading

domfournier commented Nov 28, 2022

jcapriot commented Nov 28, 2022

domfournier commented Nov 28, 2022

jcapriot commented Nov 28, 2022 •

edited

Loading

domfournier commented Nov 28, 2022 •

edited

Loading

	Now we want to refine to the maximum level, with a no padding the in `x`
	Now we want to refine to the maximum level, with a no padding in the `x`

	Whether to balance cells diagonally in the refinement, `None` implies using
	Whether to balance cells diagonally in the refinement. `None` implies using

Add new refine surface, bounding box, and point refine methods. #297

Add new refine surface, bounding box, and point refine methods. #297

Conversation

jcapriot commented Nov 23, 2022 • edited Loading

codecov bot commented Nov 23, 2022 • edited Loading

Codecov Report

jcapriot commented Nov 24, 2022 • edited Loading

domfournier commented Nov 27, 2022 • edited Loading

domfournier Nov 27, 2022

Choose a reason for hiding this comment

domfournier Nov 27, 2022

Choose a reason for hiding this comment

domfournier Nov 27, 2022

Choose a reason for hiding this comment

domfournier Nov 27, 2022

Choose a reason for hiding this comment

domfournier Nov 27, 2022

Choose a reason for hiding this comment

domfournier Nov 27, 2022

Choose a reason for hiding this comment

jcapriot Nov 27, 2022

Choose a reason for hiding this comment

domfournier commented Nov 27, 2022

jcapriot commented Nov 27, 2022 • edited Loading

domfournier commented Nov 27, 2022

dccowan commented Nov 27, 2022

jcapriot commented Nov 28, 2022 • edited Loading

domfournier commented Nov 28, 2022

jcapriot commented Nov 28, 2022

domfournier commented Nov 28, 2022

jcapriot commented Nov 28, 2022 • edited Loading

domfournier commented Nov 28, 2022 • edited Loading

jcapriot commented Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

jcapriot commented Nov 24, 2022 •

edited

Loading

domfournier commented Nov 27, 2022 •

edited

Loading

jcapriot commented Nov 27, 2022 •

edited

Loading

jcapriot commented Nov 28, 2022 •

edited

Loading

jcapriot commented Nov 28, 2022 •

edited

Loading

domfournier commented Nov 28, 2022 •

edited

Loading