-
Notifications
You must be signed in to change notification settings - Fork 6
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
Improve performance of Mesh.add method for lists #213
base: main
Are you sure you want to change the base?
Improve performance of Mesh.add method for lists #213
Conversation
34f5766
to
0b1b3fe
Compare
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.
Thanks for the nice changes!
I have a few small suggestions
if isinstance(item, Mesh): | ||
return Mesh | ||
elif isinstance(item, Function): | ||
return Function | ||
elif isinstance(item, BoundaryConditionBase): | ||
return BoundaryConditionBase | ||
elif isinstance(item, Material): | ||
return Material | ||
elif isinstance(item, Node): | ||
return Node | ||
elif isinstance(item, Element): | ||
return Element | ||
elif isinstance(item, GeometrySetBase): | ||
return GeometrySetBase | ||
elif isinstance(item, GeometryName): | ||
return GeometryName | ||
else: | ||
return type(item) |
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.
Could we simplify this construct?
For example
if isinstance(item, Mesh): | |
return Mesh | |
elif isinstance(item, Function): | |
return Function | |
elif isinstance(item, BoundaryConditionBase): | |
return BoundaryConditionBase | |
elif isinstance(item, Material): | |
return Material | |
elif isinstance(item, Node): | |
return Node | |
elif isinstance(item, Element): | |
return Element | |
elif isinstance(item, GeometrySetBase): | |
return GeometrySetBase | |
elif isinstance(item, GeometryName): | |
return GeometryName | |
else: | |
return type(item) | |
for cls in (Mesh, Function, BoundaryConditionBase, Material, Node, Element, GeometrySetBase, GeometryName): | |
if isinstance(item, cls): | |
return cls | |
return type(item) |
if base_type is Mesh: | ||
self.add_mesh(add_item, **kwargs) | ||
elif isinstance(add_item, Function): | ||
elif base_type is Function: | ||
self.add_function(add_item, **kwargs) | ||
elif isinstance(add_item, BoundaryConditionBase): | ||
elif base_type is BoundaryConditionBase: | ||
self.add_bc(add_item, **kwargs) | ||
elif isinstance(add_item, Material): | ||
elif base_type is Material: | ||
self.add_material(add_item, **kwargs) | ||
elif isinstance(add_item, Node): | ||
elif base_type is Node: | ||
self.add_node(add_item, **kwargs) | ||
elif isinstance(add_item, Element): | ||
elif base_type is Element: | ||
self.add_element(add_item, **kwargs) | ||
elif isinstance(add_item, GeometrySetBase): | ||
elif base_type is GeometrySetBase: | ||
self.add_geometry_set(add_item, **kwargs) | ||
elif isinstance(add_item, GeometryName): | ||
elif base_type is GeometryName: | ||
self.add_geometry_name(add_item, **kwargs) | ||
elif isinstance(add_item, list): | ||
for item in add_item: | ||
self.add(item, **kwargs) | ||
elif base_type is list: | ||
self.add_list(add_item, **kwargs) | ||
else: | ||
raise ( | ||
TypeError( |
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.
Again maybe directly simplify this, for example
method_map = {
Mesh: self.add_mesh,
Function: self.add_function,
BoundaryConditionBase: self.add_bc,
Material: self.add_material,
Node: self.add_node,
Element: self.add_element,
GeometrySetBase: self.add_geometry_set,
GeometryName: self.add_geometry_name,
list: self.add_list,
}
if base_type in method_map:
method_map[base_type](add_item, **kwargs)
else:
raise TypeError(f'No Mesh.add case implemented for type: "{type(add_item)}"!')
@@ -181,6 +207,46 @@ def add_geometry_name(self, geometry_name): | |||
for key in keys: | |||
self.add(geometry_name[key]) | |||
|
|||
def add_list(self, add_list, **kwargs): |
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.
Optionally you could directly add the type hints if we add/touch functions throughout the code base
check the final list for duplicate entries which is much more | ||
performant. | ||
|
||
For all other types of items, we add each element individually. |
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.
And optionally we could also directly add the args here in the docstring
For all other types of items, we add each element individually. | ||
""" | ||
|
||
types = {self.get_base_mesh_item_type(item) for item in add_list} |
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.
Why do we create a dictionary here? Would a simple list also suffice?
The common way to add objects to a
Mesh
is to use theadd
method. This allows to check that no duplicate entries exist in the mesh (if this check is really necessary can be debated). Up until now when we added a list of items, we added each element on its own via theadd
method in a recursive manner. This works fine as long as there are not too many entries in the list.However, it a list with a large number of entries is added, we run into the problem, that for each element in the list, we check if this element is already in the mesh, then extend the corresponding data structure in the mesh and then do the same thing for the next item. This scales very badly due to the repeated checking for duplicate entries. A better approach is to add the whole list to the mesh and then check in the end, if we have double elements. By doing so we only have to check for duplicate entries once and the performance is much better.
This allows do directly use the
add
method in the creation of the space time meshes, instead of the previous hack. There we can also nicely see the performance impact, as the space time performance test with using the oldadd
method did not finish after 5 minutes on my machine, but with the new method, we achieve basically the same performance as with the previous hack.