SKIRT / SKIRT9

SKIRT version 9 -- advanced radiative transfer in dusty systems
http://www.skirt.ugent.be
GNU Affero General Public License v3.0
37 stars 31 forks source link

Add Nested Density Tree Policy #196

Closed arlauwer closed 1 year ago

arlauwer commented 1 year ago

Description

Motivation Allows for using nested OctTree / BinTree for greater refinement in user chosen regions

Tests Tested for both OctTree and BinTree with different refinement levels and DensityTree properties such as maxDustFraction

petercamps commented 1 year ago

@arlauwer Thank you for this contribution. Great idea! I'd very much like to include this nice feature in SKIRT. However, at this point I have a few concerns and questions.

Your implementation breaks encapsulation (you overwrite a user-configured property from a subclass). This usually means that the class structure is improperly designed. In this case, perhaps the min/maxLevel properties should be moved to the subclass -- but I haven't thought it through yet.

A related issue is that, with the proposed implementation, it is possible to recursively nest the tree policies. This sounds fabulous but it begs the question of what we want to offer the user:

These three cases each require a fairly different class structure and implementation. Once we have decided which way to go, I will gladly do the work. What are your thoughts?

arlauwer commented 1 year ago

@petercamps I'm aware this solution isn't the most elegant. Perhaps combining minLevel and maxLevel into needsSubdivide and changing the loop in constructTree to be independent of the level could be a better approach. This way the subclass NestedDensityTreePolicy will just have to call the needsSubdivide of the current region, without checking the level.

Most people I've asked believe a strictly nested region is enough. We can however handle non-nested regions and apply the lowest tree's refinement & properties as if they were nested.

So I suggest your last option and using the same Q&A approach where the user is asked for the nested (or next) region. In case of overlap, we can prioritize the properties of the lowest region.

mbaes commented 1 year ago

@petercamps I am personally a bit hesitant to offer the maximum level of flexibility to the user because I think the nested octree grids will probably only be used in the case of a single nested region. So I would be inclined to rather go for the first option. But if it is easier and cleaner implementation-wise to go for option 3, perfectly fine for me…

petercamps commented 1 year ago

@arlauwer: I followed your suggestion for a cleaner implementation. I also adjusted the property names to be more consistent with overall naming conventions. This means you will need to manually update the property names in your current ski files; sorry about that.

@mbaes: The recommended use case is a single nested region, but it is possible to recursively nest successively smaller regions (because it was much easier to allow it than to block it). This is mentioned in the documentation as a secondary use case.