Inconsistency in look-up table conversion in cooling_cuda.cu

[brantr]

I am trying to reconcile various offline versions of the code base. One difference I noticed between a code base @bvillasen shared on June 15, 2023 and the current code main code base is in src/cooling/cooling_cuda.cu around line 344 in the main branch:

  // remap coordinates for texture
  log_T = (log_T - 1.0)*10;
  log_n = (log_n + 6.0)*10;

This appears in Bruno's version as

  // remap coordinates for texture
  log_T = (log_T - 1.0)/8.1;
  log_n = (log_n + 6.0)/12.1;

This seems different! Any idea or recollection of what may have happened here? @bvillasen @evaneschneider

[bvillasen]

seems like it was by @alwinm in this commit: https://github.com/cholla-hydro/cholla/commit/d79ed0b7e33d3d53f8a0d6d3dbd53acd8e1290fe

[evaneschneider]

Right, @alwinm rewrote the texture mapping since the old version with texture references no longer compiles for modern versions of cuda (which require texture objects). That said, I didn't think @bvillasen actually used any of the code in that source file? Or at least not the Cloudy cooling function?

[bvillasen]

That's right, I don't use the cuda_cooling module. I think that the discrepancy is because those changes were pushed to the main branch without passing through the CAAR branch. @brantr The code that I sent you is the same as the CAAR branch which includes the most up-to-date version of cosmology and chemistry_gpu. It seems like the CAAR branch has not been combined with the main branch in a while... perhaps merging both would be a good idea. Also, the CAAR branch has an FFT module that I added which would be useful for the RT FFTs, from what I saw, the RT FFTs were calling the poison solver...

[alwinm]

Just to help explain what happened, in the past the old version with texture references assumed normalized texture coordinates, which require passing in values from 0.0 to 1.0. Hence, the division by 8.1 and 12.1.

In the new version, I took out the normalization because I didn't want CUDA applying 8-bit interpolation, which was leading to surprises (errors) of O(1e-2) when I was testing the cooling module. We now directly access exact values from the table and do our own bilinear interpolation. Since input coordinates are not normalized between 0.0 and 1.0 anymore, they are instead table pixel coordinates, hence the multiplication by 10 since we have 10 values per decade in the table.

The code in Bilinear_Texture shows what goes into accessing table values. As a simple example, if you have log10(T) = 5 and log10(n) = 4,

the remapped log_T = 40 the remapped log_n = 100

this accesses table[40,100] (roughly speaking, lets ignore off-by-one, half pixel offset, etc. for now). It also accesses its neighbors, but in this example when applying interpolation the weight of the neighbors would be 0.

As a side note I am planning to rename log_T and log_n to remapped_log_T and remapped_log_n after remapping to reduce confusion.

[evaneschneider]

To clarify, last August I pulled the last of the CAAR (as in, for the CAAR project) changes into dev (and main). I believe Bruno submitted one additional PR last fall to the CAAR branch with updates to cosmology and chemistry, which we (the Pitt developers) have not touched. We have been doing our development work on dev, since that is the branch where we now have automated regression testing. I have not had the person-power to have someone pull Bruno's final CAAR PR into dev since October, since we don't have any experience on our side running the cosmology code.

I believe this issue can be closed since your initial question is resolved, @brantr?

[brantr]

OK, that's fine. Closing.

Yes, Bruno, I am integrating src/fft/ into Nick's RT fork.