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

Enable 5D grids #422

Open
dmholtz opened this issue Mar 19, 2024 · 2 comments
Open

Enable 5D grids #422

dmholtz opened this issue Mar 19, 2024 · 2 comments

Comments

@dmholtz
Copy link
Contributor

dmholtz commented Mar 19, 2024

Recently, I worked successfully with 5D hash grids of tinycudann by removing the comment in

// case 5: return new GridEncodingTemplated<T, 5, N_FEATURES_PER_LEVEL, HASH_TYPE>{ TCNN_GRID_PARAMS };

To streamline the setup process, I would like to get rid of my private fork and have 5D grids directly enabled in the official tinycudann codebase.

@Tom94 do you have any objections against this? If not, I will prepare a PR which removes the comment in the line and adapts the documentation.

@Tom94
Copy link
Collaborator

Tom94 commented Mar 19, 2024

Hi, the reason some of the dimensionalities are commented out is that they increase the compile time and memory cost by quite a lot -- for a feature that most people are not going to use.

But I hear your preference for upstreaming this. How about making the enabled hash encoding dimensions a preprocessor define? I am thinking TCNN_HASH_MIN_DIM (with default 2) and TCNN_HASH_MAX_DIM (with default 4). Those variables could then be overridden via CMake options -- see the top of CMakeLists.txt for how existing options are defined and then turned into preprocessor defines via target_add_definitions.

For the Python bindings, we could have an additional setup.py argument, similar to how --no-networks is currently handled, that similarly gets turned into TCNN_HASH_MIN_DIM and TCNN_HASH_MAX_DIM.

@dmholtz
Copy link
Contributor Author

dmholtz commented Mar 25, 2024

Thank you very much for your reply. I agree that preporcessor defines would be the cleanest approch to balance feature scope and compile time for all users of tcnn.

In principle, I would be interested in making such a contribution. However, I cannot promise ATM when I find time to look into this in more detail. Until then, I will stick to the fork-solution and revist this issue once I start refactoring my own code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants