cholla-hydro / cholla

A GPU-based hydro code
https://github.com/cholla-hydro/cholla/wiki
MIT License
60 stars 32 forks source link

Reconstruction Kernel Fusion 1: Convert to structs #371

Closed bcaddy closed 1 month ago

bcaddy commented 4 months ago

Summary

The goal of this PR is a first step in preparing to fuse the reconstruction and Riemann solver kernels. To do this it converts many variables in both the reconstructions and the Riemann solvers into structs. Most of these structs were already present in the reconstruction and mhd namespaces but they have been generalized and moved into the hydro_utilities namespace in the basic_structs.h file and the reconstruction namespace in reconstruction.h.

Given the number of changes and the new structure of data I would like everyone to at least glance through the contents of basic_structs.h and reconstruction.h to see if it looks good to them since I plan on using those structs throughout the reconstructors.

Structs

hydro_utilitiies::Primitive

Contains all the primitive variables and the specific versions of the gas energy and scalars. Included a default and non-default constructor, mostly the non-default constructor is for tests since the previous usage of this struct in tests had ABI dependencies; the new constructor provides a more flexible API to shield the internal arrangement of the struct.

hydro_utilitiies::Conserved

Contains all the conserved variables and the non-specific versions of the gas energy and scalars. Also has a constructor for the reasons listed above.

hydro_utilitiies::Vector

This just has three members, x, y, and z, and is intended to serve as an easy way to move vector quantities around. I don't love the name since it sort of conflicts with std::vector but they're in different namespaces and this one stores actual mathematical vectors. This struct is used in both the Primitive and Conserved structs to store their vector quantities. I would like some feedback on naming, usefulness, etc of this struct.

reconstruction::InterfaceState

Contains all the variables used in the Riemann solver interface states. It's basically all the primitive and conserved variables together in one struct.

Renamed reconstruction.h to reconstruction_internals.h

In the final version I want a file called reconstruction.h that only has the Riemann solver facing code and a separate one that has all the code that is shared between the reconstructors. This is why I renamed reconstruction.h and created a new reconstruction.h that contains only the InterfaceState struct at the moment.

bcaddy commented 1 month ago

I'm happy to make this change. I do want to point out that except for the single Riemann solver that uses the bulk quantities, in every other place used it is the specific version of the gas energy and scalar which is why I chose those names. Do you still want me to change it?

evaneschneider commented 1 month ago

Yes, we had a discussion about it and everyone agreed that unless it is actually true everywhere, it's better to leave the name more ambiguous and comment every time the variable is first used.

evaneschneider commented 1 month ago

As I believe Helena pointed out, it is also true that in the future there may be additional versions of scalars where it makes more sense not to use the specific version, but we don't necessarily to want to add a whole additional variable to the struct, when part of the point is to streamline/abstract things.

bcaddy commented 1 month ago

Fair enough, I'll remove it.

bcaddy commented 2 weeks ago

Done, see PR #397