search

xcompact3d / x3d2

https://xcompact3d.github.io/x3d2
BSD 3-Clause "New" or "Revised" License
3 stars 4 forks source link

Refactor `transeq_cuda_dist` #75

Closed semi-h closed 5 months ago

semi-h commented 5 months ago

The PR Just simplifies the way we carry out the transeq. @Nanoseb implemented exactly in this way on the OpenMP backend which is better and I'm just following it.

The important part for the OpenMP backend is that there is actually a minor bug. I realised this only after running the TGV case on the CUDA backend, its a very subtle bug, and did the fix on the CUDA backend so that TGV works fine now.

Here is the problem. The transeq_dist_component simplifies the way we pass u, v, and w fields in different orders to correctly evaluate all the derivatives. But just in the line below, the halo components of the u, v, and w fields are not passed down to the transeq_dist_component, instead assumed that they're always u and v. https://github.com/xcompact3d/x3d2/blob/e916fe8ff835aae420193a2b3b3a1474dafc5038/src/omp/backend.f90#L221-L222

So, I'm happy to fix this quickly on the OpenMP backend as well, if you're happy with this @Nanoseb. Or if you anticipate a merge conflict due to the PR you're working on or want to fix this later its completely fine, I'll leave it as is.

Nanoseb commented 5 months ago

yes, sure. Feel free to do it. I'll handle the conflict if there is one (though I don't expect it)

slaizet commented 5 months ago

To discuss: shall we have a test for transeq calculations (similar to a test for the Poisson solver)? I do not think that we need a test for the divergence and gradient calculations. It might be useful when expending the backend options.

Nanoseb commented 5 months ago

To discuss: shall we have a test for transeq calculations

Actually we already have some, but we should probably look into why they didn't detect this issue.

semi-h commented 5 months ago

To discuss: shall we have a test for transeq calculations

Actually we already have some, but we should probably look into why they didn't detect this issue.

There was a minor error in the test as well, all fixed now.

I think the PR is now all ready. Because this PR only refactors a subroutine and doesn't really add or remove a feature, I guess we don't need to wait until Monday to discuss. Its a small PR so I'm looking for one approve, please review if you have time.