cholla-hydro / cholla

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

HDF5 Refactor + Read_Cat + ScopedTimer #287

Closed alwinm closed 1 year ago

alwinm commented 1 year ago

Major breaking change: the scalar loop in hdf5 output from here on will be invalid, all advected scalars should have their own names (rather than scalar1, scalar2, etc.), in order to avoid awful nested ifdef contraptions.

Refactor HDF5 input/output for:

Add init=Read_Grid_Cat function for restarting from a concatenated file (WARNING: this is slower on most systems, though performance of reading only occurs once, so currently not worth worrying about. This function should be updated by anyone actually using it to include desired fields)

Add ScopedTimer timing utility as a one-liner for timing a function. (See Grid3D::Read_Grid_Cat for example use). Does "nothing" without CPU_TIME.

alwinm commented 1 year ago

In this rendition I held off on deciding what to do with the PRINT_INITIAL_STATS flag.

It would be easy to:

1) Support it by editing the read_grid functions 2) Remove it by deleting associated variables under MHD.

alwinm commented 1 year ago

In this rendition since some of the Chemistry fields are host/cpu-only, I left all of them on the cpu-side. But if desired I believe I could gpu-accelerate all the Grid.Conserved chemistry quantities.

To possibly remove needing to cudamemcpy the grid during output steps, we would need to port all the projection/slice/rotated projection code. Which is likely worth doing in a future PR.

bcaddy commented 1 year ago

Major breaking change: Scalar loop from here on will be invalid, all advected scalars should have their own names (rather than scalar1, scalar2, etc.), in order to avoid awful nested ifdef contraptions.

Could you expand on this a bit? We have a lot of loops over scalars in pretty much every kernel.

alwinm commented 1 year ago

Major breaking change: Scalar loop from here on will be invalid, all advected scalars should have their own names (rather than scalar1, scalar2, etc.), in order to avoid awful nested ifdef contraptions.

Could you expand on this a bit? We have a lot of loops over scalars in pretty much every kernel.

Good point. I misspoke. Scalar loop in hdf5 output will not be valid. Everywhere else, no change.

bcaddy commented 1 year ago

Got it, thank you for clarifying

evaneschneider commented 1 year ago

This looks good to me, although I'd like @bcaddy to confirm that the array sizes / buffers for MHD reads and writes are correct. Does this also mean that this commit breaks the scalar loop in e.g. the slice outputs? Or was that already broken because it doesn't reference basic scalar?

alwinm commented 1 year ago

This looks good to me, although I'd like @bcaddy to confirm that the array sizes / buffers for MHD reads and writes are correct. Does this also mean that this commit breaks the scalar loop in e.g. the slice outputs? Or was that already broken because it doesn't reference basic scalar?

That's a good point--slice/projections will behave as before, but I believe we should phase out the scalar loops therein.

evaneschneider commented 1 year ago

I will merge this once @bcaddy confirms that the buffers etc. are good for MHD.

bcaddy commented 1 year ago

I totally forgot about this, sorry! I'll start on it now.