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.

Sign up for GitHub

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

Jump to bottom

Implement BIH tree #849

Merged
merged 64 commits into from
Aug 8, 2023
Merged

Implement BIH tree #849

merged 64 commits into from
Aug 8, 2023

Conversation

elliottbiondo
Copy link
Contributor

This PR adds the ability to construct a bounding interval hierarchy (BIH) tree from volume bounding boxes. Bounding boxes are read from .json or populated during VolumeInput construction. These bounding boxes are stored on VolumeRecords. When each universe is constructed (via UnitInserter), a BIH tree is created using BIHMaker class. A future PR will involve hooking this tree up to SimpleUnitTracker, to speed up point-in-volume, and move-across-surface calls.

@sethrj sethrj self-requested a review July 10, 2023 20:11
@sethrj sethrj added enhancement New feature or request orange Work on ORANGE geometry engine labels Jul 10, 2023
sethrj
sethrj reviewed Jul 10, 2023
View reviewed changes
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments before digging too deep into the BIH construction.

src/orange/construct/OrangeInputIO.json.cc Outdated Show resolved Hide resolved
src/orange/Types.hh Outdated Show resolved Hide resolved
src/orange/OrangeData.hh Outdated Show resolved Hide resolved
src/orange/OrangeData.hh Outdated Show resolved Hide resolved
src/orange/OrangeData.hh Outdated Show resolved Hide resolved
src/orange/detail/BIHMaker.cc Outdated Show resolved Hide resolved
src/orange/detail/BIHMaker.cc Outdated Show resolved Hide resolved
src/orange/detail/BIHMaker.cc Outdated Show resolved Hide resolved
src/orange/detail/BIHMaker.cc Outdated Show resolved Hide resolved
test/orange/detail/BIHMaker.test.cc Outdated Show resolved Hide resolved
@sethrj sethrj marked this pull request as draft July 13, 2023 14:40
@sethrj
Copy link
Member

sethrj commented Jul 13, 2023

Converting to draft per our discussion

So w.r.t. my Celeritas BIH PR: I put that in essentially as a prototype. I figured I would get that in and then see what I could do with getting around the dynamic memory allocation when querying the tree. Some of your comments --- like having separate structs for inner and leaf nodes, making a BBoxUtils class, etc --- are a "final cleanup" sort of things. I am not sure it worth spending time cleaning up BIH trees until we are certain that's what we are going with (rather than switch to k-d trees perhaps). Maybe I should keep pushing forward with querying BIH trees before cleaning up the PR?

@elliottbiondo
Rename BIHMaker to BIHBuilder
8394aab
@elliottbiondo
Fix axis comparison and make methods const
082e493
@elliottbiondo
Clean up types
6db249e
@elliottbiondo
Moving BIHNodeId to OrangeTypes
328cee6
@elliottbiondo
Create BIHParams struct
8ba6583
@elliottbiondo
Fix reading bbox from JSON
2d760c6
@elliottbiondo
Remove bitwise operation
f9a526d
@elliottbiondo
Rename bbox function to is_infinite
159ed7d
@elliottbiondo
Add BoundingBoxUtils and tests
3bb3a5c
@elliottbiondo
Fix doc string
470b1b0
@elliottbiondo
Create separate partitioner class
d66c9d9
@elliottbiondo
Store parent on each BIHNode
a824ba8
@elliottbiondo
Move more functionality to BIHPartitioner
1cb58a6
@elliottbiondo
Implement surface area heuristic partitioning scheme
b24e63f
@elliottbiondo
Clean up doc strings
6c86191
@elliottbiondo
Stop storing all partition candidates
e193a6a
@elliottbiondo
Build BIH tree in terms of inner and leaf node structs
dc48e5a
@elliottbiondo
Change appropriate EXPECTs to ASSERTs
594e1eb
@elliottbiondo
Add uniquify tolerance
b82b08d
@elliottbiondo
Move BIHData to separate file
7f61028
@elliottbiondo
Copy link
Contributor Author

@sethrj this is ready for another look

@sethrj sethrj marked this pull request as ready for review July 26, 2023 12:17
@elliottbiondo
Copy link
Contributor Author

@sethrj this is ready for another look

@sethrj sethrj marked this pull request as ready for review August 3, 2023 13:04
@sethrj
Copy link
Member

sethrj commented Aug 3, 2023

@elliottbiondo Can you separate the bbox changes into another PR? I can show you some tricks that can make the construction of an incremental PR easier.

@sethrj sethrj mentioned this pull request Aug 4, 2023
Merged
sethrj pushed a commit that referenced this pull request Aug 6, 2023
@elliottbiondo
Refactor and extend BoundingBox (#879)
59ef6f2 
* Template BoundingBox class on real type
* Fix bbox reading and add support for converting "max" to "infinity"
* Define bounding box utilities

(Split from #849)
@sethrj
Copy link
Member

sethrj commented Aug 6, 2023 •
edited
Loading

@elliottbiondo I merged the bbox changes from upstream into this branch and will finish reviewing now. Thanks for your patience!

sethrj
sethrj requested changes Aug 7, 2023
View reviewed changes
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good @elliottbiondo! Just a couple of comments.

src/orange/detail/BIHData.hh Outdated Show resolved Hide resolved
src/orange/BoundingBoxIO.json.hh Outdated Show resolved Hide resolved
src/orange/BoundingBox.hh Outdated Show resolved Hide resolved
src/orange/detail/BIHData.hh Outdated Show resolved Hide resolved
src/orange/detail/BIHData.hh Outdated Show resolved Hide resolved
src/orange/detail/BIHBuilder.cc Outdated Show resolved Hide resolved
src/orange/detail/BIHBuilder.cc Outdated Show resolved Hide resolved
src/orange/detail/BIHBuilder.hh Outdated Show resolved Hide resolved
src/orange/detail/BIHPartitioner.cc Outdated Show resolved Hide resolved
src/orange/detail/BIHBuilder.hh Show resolved Hide resolved
@elliottbiondo
Copy link
Contributor Author

@sethrj this should be good to go

sethrj
sethrj reviewed Aug 8, 2023
View reviewed changes
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the last set of changes to Real3 etc. was in response to "use doubles while constructing and floats for storing"? I'm fine with that decision.

src/orange/BoundingBoxIO.json.hh Outdated Show resolved Hide resolved
src/orange/BoundingBox.hh Outdated Show resolved Hide resolved
sethrj
sethrj approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @elliottbiondo !

@elliottbiondo
Copy link
Contributor Author

@sethrj assuming the trailing return type is okay, this is ready

sethrj
sethrj reviewed Aug 8, 2023
View reviewed changes
src/orange/BoundingBox.hh Outdated Show resolved Hide resolved
@sethrj sethrj merged commit fbd562c into celeritas-project:develop Aug 8, 2023
@sethrj sethrj mentioned this pull request Aug 8, 2023
Merged
sethrj pushed a commit that referenced this pull request Aug 16, 2023
@stognini
Hotfix: fix missing include from #849 (#898)
e16e928
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sethrj sethrj sethrj approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request orange Work on ORANGE geometry engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elliottbiondo @sethrj