Skip to content
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

Add Nested Density Tree Policy #196

Merged
merged 5 commits into from
Oct 4, 2023
Merged

Add Nested Density Tree Policy #196

merged 5 commits into from
Oct 4, 2023

Conversation

arlauwer
Copy link
Contributor

@arlauwer arlauwer commented Oct 2, 2023

Description

  • slight changes to DensityTreePolicy & TreePolicy to conform with the new NestedDensityTreePolicy
  • added extra contains & intersects to Box

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

arlauwer and others added 2 commits September 30, 2023 16:07
- changed DensityTreePolicy & TreePolicy consequently
- added extra contains & intersects to Box
@petercamps
Copy link
Contributor

@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:

  • A single subregion of the grid domain that has different subdivision parameters.
  • Recursively nested subregions (logically and spatially) each with their subdivision parameters (each region must be contained in the region "above" it; the parameters of the deepest nested region are used).
  • Multiple subregions that may be scattered around the overall grid domain, with some rule of precedence of which parameters are used in case of overlap. This is the option with maximum flexibility.

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

arlauwer commented Oct 3, 2023

@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
Copy link
Contributor

mbaes commented Oct 3, 2023 via email

@petercamps
Copy link
Contributor

@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.

@petercamps petercamps merged commit 6f61d94 into SKIRT:master Oct 4, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants