Moved omega from an attribute of the collision to the input of its callable #100
Conversation
hsalehipour
force-pushed
the
omega_stepper_input
branch
from
ec707cb to
72208e3
Compare
|
Is there any practical purpose for this change or is cosmetic? Viscosity of a fluid cannot change physically in the basic Incompressible Navier Stokes flow. If there are any simulations that support this feature (e..g., viscosity change due to thermal, etc.) it should be another solver with its own stepper function that supports this. It's best to avoid adding buffers to kernels due to performance when ideally and most likely constants, then the compiler can better optmize the kernel. |
hsalehipour
force-pushed
the
omega_stepper_input
branch
from
72208e3 to
65f6d45
Compare
|
Thanks for the feedback. No it is not just a cosmetic change. The main reason that prompted me to add this is for ML applications where for instance one wants to create various trajectories of the solution at different values of viscosity. We should not need to re-create the stepper functions for a new parameter. Again all other problem configurations are correctly set up to be independent from the stepper except for omega. Another example is for computational stability reasons: sometimes you mean gradually increase the Reynolds number to arrive at the actual Re. |
|
I agree with your comment about adding constant buffers to kernels. I have made omega static in both warp and jax which should address your comment. But even without making them static, MLUPS numbers were the same as before. |
hsalehipour
force-pushed
the
omega_stepper_input
branch
from
65f6d45 to
d8d5bcd
Compare
|
@mehdiataei I am really not sure if the use of wp.static in my latest forced commit does what I intended. Anyway please take a look. Regardless I am fine removing wp.static because MLUPs are not affected. |
examples/cfd/flow_past_sphere_3d.py
Outdated
|
|
||
| # Ensure warp type is set correctly for Omega | ||
| if self.backend == ComputeBackend.WARP: | ||
| self.omega = wp.static(self.precision_policy.compute_precision.wp_dtype(self.omega)) |
There was a problem hiding this comment.
The casting should be done in the kernels not here.
There was a problem hiding this comment.
See other primitive values casting in the kernels
There was a problem hiding this comment.
Agreed. fixed it.
examples/cfd/lid_driven_cavity_2d.py
Outdated
| @@ -29,6 +29,10 @@ def __init__(self, omega, prescribed_vel, grid_shape, velocity_set, backend, pre | |||
| self.boundary_conditions = [] | |||
| self.prescribed_vel = prescribed_vel | |||
|
|
|||
| # Ensure warp type is set correctly for Omega | |||
| if self.backend == ComputeBackend.WARP: | |||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| omega: float, | ||
| velocity_set: VelocitySet = None, | ||
| precision_policy=None, | ||
| compute_backend=None, | ||
| ): |
There was a problem hiding this comment.
Remove the extra line after ):
xlb/operator/collision/kbc.py
Outdated
| velocity_set=velocity_set, | ||
| precision_policy=precision_policy, | ||
| compute_backend=compute_backend, | ||
| ) | ||
|
|
||
| @Operator.register_backend(ComputeBackend.JAX) | ||
| @partial(jit, static_argnums=(0,), donate_argnums=(1, 2, 3)) |
hsalehipour
force-pushed
the
omega_stepper_input
branch
3 times, most recently
from
883a199 to
08e08d1
Compare
|
Pleaes check again. I addressed all comments. |
hsalehipour
force-pushed
the
omega_stepper_input
branch
from
08e08d1 to
7fa179f
Compare
xlb/operator/collision/kbc.py
Outdated
| @@ -74,13 +71,17 @@ def jax_implementation( | |||
| else: | |||
| raise NotImplementedError("Velocity set not supported: {}".format(type(self.velocity_set))) | |||
|
|
|||
| # Compute required constants based on the input omega (omega is the inverse relaxation time) | |||
| beta = self.compute_dtype(0.5 * omega) | |||
There was a problem hiding this comment.
You must apply compute dtype to both numbers separately before multipication
xlb/operator/collision/kbc.py
Outdated
| @@ -278,6 +278,10 @@ def functional( | |||
| shear = decompose_shear_d2q9(fneq) | |||
| delta_s = shear * rho / self.compute_dtype(4.0) | |||
|
|
|||
| # Compute required constants based on the input omega (omega is the inverse relaxation time) | |||
| _beta = self.compute_dtype(0.5 * omega) | |||
… to an input of the methods.
hsalehipour
force-pushed
the
omega_stepper_input
branch
from
7fa179f to
c87b3a5
Compare
|
Thanks looks great |
Contributing Guidelines
Description
Moved omega from an attribute of the collision and stepper operation to an input of the methods. Viscosity (or omega) should not be an attribute of the collision or stepper object but rather an input to these methods. For example if you want to change viscosity during runtime after n iterations, you should not need to create another stepper object! Currently, we have all simulation setup configuration and parameters (BC, IC, geometry) seprated from the stepper except for Omega. This PR aims to fix that.
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 .