Multi-res grid refinement + Neon backend support#159
Conversation
Fixed KBC bug
* Fixed some runtime bugs * fixed some naming/spelling errors * removed some debugging comments. * Introduced a new file `cell_type.py` containing boundary-mask constants for fluid voxelss to replace hardcoded values with the new constants. * Applied renaming of 254 to SFV to function names
- Unified multi-resolution recursion builder in `simulation_manager.py` to streamline the construction of simulation steps. - Refactored nse_multires_stepper for improved clarity - Updated performance optimization handling in `multires_momentum_transfer.py` to support multiple fusion strategies.
…ine and clarify the implementation of multi-resolution streaming steps.
(refactoring) Cleaning up multi-res stepper.
Documentation
… multi-res by ensuring consistent use of `store_dtype` and `compute_dtype`.
Fixed mixed precision handling of the Neon backend
…ed + README update (#39)
* (build) Introducing Neon backend as an optional installation parameter. * (install) new installation mode for neon backend. * (build) Add ARM support for Neon wheel resolution * (documentation) Fixes to README and AUTHORS * (ruff) fixes to the style * (documentation) fix list of supported python versions
mehdiataei
left a comment
There was a problem hiding this comment.
Thank you @massimim, this looks like a strong contribution. I left code-level comments in this round, but I also wanted to share a few higher-level suggestions.
- I think framing Neon as a new compute "backend" may be slightly misleading. Conceptually, Neon here does not seem fully parallel to JAX. The implementation largely reuses Warp functionals and then executes them through Neon handles, containers, and skeletons. In that sense, Neon feels more like an execution/runtime layer on top of Warp code generation than a standalone compute backend.
I think a different framing could make the design clearer:
- If Neon is fundamentally "Warp math + Neon execution", I would be hesitant to model it as a third peer backend throughout the operator hierarchy.
- Instead, I would consider splitting the abstraction into:
- kernel / math backend:
JAXvsWarp - execution runtime: direct Warp launch vs Neon container/skeleton launch
- kernel / math backend:
I think this would make Neon more generic and would better highlight its real strength: the execution model and skeleton abstraction, rather than presenting it as a bespoke backend. It may also make the integration easier to extend and adopt. Several of the current issues feel like symptoms of the abstraction boundary being one layer off. This likely needs some careful design thought, but I would strongly encourage it. To me, the more compelling framing is that Neon provides a skeleton/runtime that Warp kernels can target.
- The multires implementation also feels too monolithic. Kernels, schedule planning, state ownership, and runtime graph compilation all live in roughly the same layer, which makes the system harder to reason about, test, and extend.
One possible improvement would be to introduce a typed MultiresPlan / Schedule layer that represents the recursive timestep as explicit operations, then have a separate Neon graph builder that lowers that plan into containers/skeletons. I would also keep simulation state in a manager and keep kernels separate from schedule construction.
- The topology and coordinate model feels too implicit at the moment, which makes it harder to debug and reuse. An explicit
MultiresTopologyorLevelInfoabstraction could help a lot, with methods such as:
level_shape(level)global_bounds(level)to_global(level, coords)face_indices(level, side)active_indices(level)
I think making those concepts explicit would improve both clarity and correctness.
- For the new BC, I suggest clearly clarifying the Re that it has been validated for.
| velocity_set = velocity_set or DefaultConfig.velocity_set | ||
| if compute_backend == ComputeBackend.WARP: | ||
| from xlb.grid.warp_grid import WarpGrid | ||
|
|
There was a problem hiding this comment.
you can delete WarpGrid import
|
Hi @mehdiataei , thank you for the review. This PR is the result of quite a long journey with @hsalehipour and others too. I'll go through points 1 to 3.
@hsalehipour and I are addressing the remaining comments and will send a PR revision. |
…#41) * (build) Introducing Neon backend as an optional installation parameter. * (install) new installation mode for neon backend. * (build) Add ARM support for Neon wheel resolution * (documentation) Fixes to README and AUTHORS * (ruff) fixes to the style * (documentation) fix list of supported python versions * (install) Add installation unit tests for JAX, Warp and Neon backends, update utils for Warp to JAX conversion
…ion (#42) * (build) Introducing Neon backend as an optional installation parameter. * (install) new installation mode for neon backend. * (build) Add ARM support for Neon wheel resolution * (documentation) Fixes to README and AUTHORS * (ruff) fixes to the style * (documentation) fix list of supported python versions * (install) Add installation unit tests for JAX, Warp and Neon backends, update utils for Warp to JAX conversion * (install) Enhance warp-lang uninstallation process for Neon installation - Updated `_uninstall_warp_lang` function to include a reason for uninstallation. - Modified `InstallWithNeonHooks` class to uninstall `warp-lang` before and after installation when the `[neon]` extra is requested. - Added a new test to verify that pre-existing `warp-lang` is uninstalled during the editable install of XLB with the `[neon]` extra.
* (build) Introducing Neon backend as an optional installation parameter. * (install) new installation mode for neon backend. * (build) Add ARM support for Neon wheel resolution * (documentation) Fixes to README and AUTHORS * (ruff) fixes to the style * (documentation) fix list of supported python versions * (install) Add installation unit tests for JAX, Warp and Neon backends, update utils for Warp to JAX conversion * (install) Enhance warp-lang uninstallation process for Neon installation - Updated `_uninstall_warp_lang` function to include a reason for uninstallation. - Modified `InstallWithNeonHooks` class to uninstall `warp-lang` before and after installation when the `[neon]` extra is requested. - Added a new test to verify that pre-existing `warp-lang` is uninstalled during the editable install of XLB with the `[neon]` extra. * (cleaning) removed unnecessary 'pass' call * (refactor) Update JSON data structure and clean up code - Modified the `ahmed.json` file to ensure consistent formatting of velocity and height data. - Removed a hardcoded comment in `multires_grid.py` regarding device initialization. - Commented out the `info_print` call in `neon_grid.py` to reduce output clutter. - Enhanced the `_create_constant_prescribed_profile` method in `bc_halfway_bounce_back.py` to better handle different compute backends and added error handling for unsupported backends.
* added missing neon functionals and fixed forced_collision * fixed global coordinate calculations in NeonMultiresGrid for improved bounding box face detection across levels * Remove deprecated BC from README * fixed an issue in setting prescribed values in zouhe bc * Fixes to the PR comments (#45) * fix(install): add h5py when the neon installation option is selected. * refactor(mres): changed 'omega' to 'coalescence_factor' in function signatures. * refactor(grid): change default values of sparsity_pattern_list and sparsity_pattern_origins to None * fix(docs): correct typos in multires_flow_past_sphere_3d.py and update NEON backend description in compute_backend.py * ruff --------- Co-authored-by: massimim <57805133+massimim@users.noreply.github.com>
|
Thanks a lot, @mehdiataei , for the thorough review. We have now addressed the remaining issues in the latest commits, and the changes are ready to be merged. |
|
Hi @hsalehipour , maybe I’m missing something, but are there any benchmarks reported for this implementation? For example, DrivAer at the reference Reynolds number of 4.87M. I think this is important, since people may use this for industrial applications. If those benchmarks cannot be passed, it would be helpful to clearly document the limits of the method in an MD file inside the example folder, along with the multi-resolution example and the relevant explanations. Also, there are a number of WIP/debugging commits that should be squashed. Could you please clean those up? |
|
@mehdiataei Agreed. The new example multires_windtunnel_3d already serves that purpose and relies on ahmed.json for reference data. We attempted to squash just the WIP commits via interactive rebase. The squash itself worked, but replaying the ~190 commits that sit above the squash range produced merge conflicts in the upper commits, and we aborted to keep the branch intact. Resolving those conflicts is feasible but non-trivial, so we're proposing simple merge instead as we did for the OOC PR. |
|
What is the Reynolds number range within which the BC is intended to operate? I believe this should be explicitly clarified, particularly since the BC was previously presented as a novel design. In addition, I suggest either add a README file explaining the validity and add the tables etc or cite the appropriate literature on the method. Merging such a large number of commits (269 commits while the full library history is just 369 commits) into one PR is fairly unconventional and also risks reducing the visibility of substantial prior contributions by pushing them further back in the project history. We can also fix the OCC merge if that's your concern. Let me know what you think. |
|
If the concern is primarily around conflict resolution or preserving the existing branch state, I would be happy to help with the squashing/rebase process to make the PR history cleaner and more manageable. Please let me know! |
|
I don’t think it would be appropriate to provide empirical bounds on the stability or accuracy of HybridBC, as these characteristics are highly problem- and geometry-dependent. Based on the experiments conducted so far, the method has demonstrated strong stability. Any discrepancies with reference data are likely influenced by other important modeling components that are not yet incorporated into the library, such as wall models. Regarding the commit history, Max and I, as maintainers of the library, have decided to follow a squash-and-merge strategy for these integrations in order to keep the history concise and manageable. It would be appreciated if the same approach could also be applied retroactively to the previous OOC merge. |
Contributing Guidelines
Description
Grid refinement capability is now supported in XLB through the Neon backend. The Neon backend provides full support for dense grids on multi-GPU systems, as well as multi-resolution grids on single GPUs. All newly introduced functionalities have been carefully tested and optimized. This represents a major enhancement to the library and involves substantial additions and improvements to the codebase.
Type of change
How Has This Been Tested?
Linting and Code Formatting
Make sure the code follows the project's linting and formatting standards. This project uses Ruff for linting.
To run Ruff, execute the following command from the root of the repository:
ruff check .