-
Notifications
You must be signed in to change notification settings - Fork 17
Substructure: Atom data and boolean tree + tests #73
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
base: main
Are you sure you want to change the base?
Conversation
|
| Filename | Overview |
|---|---|
| src/substruct/atom_data_packed.h | Introduces bit-packed atom data structure (128 bits) with getters/setters for efficient GPU comparison. Includes branchless matching functions for atom and bond queries. |
| src/substruct/boolean_tree.cuh | Implements boolean expression tree evaluation for atom matching with logical operators (AND/OR/NOT), field comparisons, and recursive pattern matching using stack-based evaluation. |
| tests/test_boolean_tree.cu | Comprehensive unit tests for boolean tree evaluation covering logical operations, bond matching, and recursive patterns. Tests both host and device execution with various atom types. |
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.
Additional Comments (1)
-
tests/test_boolean_tree.cu, line 1025 (link)style: Consider adding tests for the comparison operations (
GreaterThan,LessEqual,GreaterEqual,Range) defined inboolean_tree.cuh. These operations are implemented but currently untested.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
7 files reviewed, 1 comment
| /// @name Compacted 4-bit field masks and bit positions | ||
| /// @{ | ||
| static constexpr int k4BitMask = 0x0F; ///< Mask for 4-bit fields (max value 15) | ||
| static constexpr int kNumRingsBits = 0; ///< Low 4 bits of byte 1 |
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.
I notice for several atom properties (numRings, ringBondCount, numImplicitHs, numHeteroatomNeighbors), we're using 4-bit fields with a max value of 15. Should we add bounds checking or saturation in the setters? Currently, values > 15 are silently truncated, which could cause unexpected behavior.
evasnow1992
left a comment
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.
Thank you for adding packed atom data and boolean tree operations. Only one minor comment. Also may considering adding some edge case tests like for empty trees, and/or tests for comparison operations like GreaterThan. Not required though
First PR for substructure search. Includes definitions for bit-packed atom data and comparison functions, and an implementation of a boolean tree walk.
The boolean tree reference recursive bits that are not yet implemented, but can be tested with dummy data.