Update BreezeReactantExt to enable ReactantState on LatitudeLongitudeGrid#566
Update BreezeReactantExt to enable ReactantState on LatitudeLongitudeGrid#566dkytezab wants to merge 15 commits into
BreezeReactantExt to enable ReactantState on LatitudeLongitudeGrid#566Conversation
…ibility Made-with: Cursor
…eGrid Made-with: Cursor
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com>
|
@dkytezab can you fix the conflicts? also we should add sharding tests.. maybe we want to wait until after this goes in? |
|
The new tests seem to be very expensive, might be better to move them to separate files for better load balancing. Also, maybe we want to move all the Reactant tests to a |
|
I agree with @giordano. I also wonder if there will be cases where we want to selectively test without the Reactant tests or only with the Reactant tests |
|
Reactant tests at the moment are run only in a single job because they're already very expensive |
|
Oh, sorry, I thought I was replying to Greg, of course I know you know well how they're currently run 😄 |
* Add FD validation tests for Centered and WENO compilation Adds finite-difference gradient verification to both reactant compilation test files. AD gradients are checked against one-sided FD at two grid cells and two step sizes (1e-4, 1e-6) with a 1% relative tolerance. Also removes FT type aliases and cleans up test structure. Made-with: Cursor * Use device_arch for FD validation instead of hardcoded CPU FD validation grids and models now use the same architecture as the Reactant backend (GPU or CPU), so the test runs on the actual device. Made-with: Cursor * Apply suggestions from code review Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com> * What kind of illiteracy is this - ken * Lower rtol * Formatting fixes --------- Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com>
|
@dkytezab the rebase went wrong 🥲 |
Made-with: Cursor
|
claude got a little uppity./.. but i think now this is ok? cleaning this up atm |
| const ReactantLatLonModel = AtmosphereModel{<:Any, <:Any, <:ReactantState, <:Any, <:LatitudeLongitudeGrid} | ||
|
|
||
| function AtmosphereModels.set_product!(dest::Field{<:Any, <:Any, <:Any, <:Any, <:LatitudeLongitudeGrid}, a, b) | ||
| @jit set!(dest, a * b) |
There was a problem hiding this comment.
@Pangoraw @wsmoses is this the best approach? @jit causes MLIR re-compilation in every single call, that doesn't sound ideal.
Also, the signature of this method is effectively type piracy, did you mean to use the ReactantLatLonModel defined above? That's entirely unused.
There was a problem hiding this comment.
I believe this is only called during initialization (eg in set!) @giordano I agree it should be moved to Oceananigans.
There was a problem hiding this comment.
what fails if you don't do this?
@dkytezab can answer that I think
There was a problem hiding this comment.
this was a workaround for setting velocity fields outside of a compiled body... which works fine on RectilinearGrid but not LatitudeLongitudeGrid
There was a problem hiding this comment.
granted we can just only include set! in a compiled body but there's then a surprising asymmetry between the two grids
There was a problem hiding this comment.
@dkytezab more specifically it is an issue with setting BinaryOperation -- right?
4 participants
set!called on concrete arrays. See Add LatitudeLongitudeGrid support with CompressibleDynamics #520.