Skip to content

Conversation

@J-Hizzle
Copy link
Contributor

Part of #47.

Adds functionality to linearly interpolate data contained in a MultiHistogram from a coarse grid to a fine grid and includes a new function to wipe data of a MultiHistogram outside a symmetric slab.

I did not yet take care of the concerns expressed by GitHubCopilot in #49 (comment) and #49 (comment), so this pull request is not ready for merging yet.

@J-Hizzle
Copy link
Contributor Author

I addressed Copilot's comment in #49 (comment) in commit 0704aea. Now, it is asserted that the given target grid and the grid of the output histogram are the same.

@J-Hizzle
Copy link
Contributor Author

Concerning comment #49 (comment):

[nitpick] The interpolation factor calculation may be incorrect. On line 48, the factor is calculated as (grid(binIdx) - inputGrid(leftBin)) * input.inverseBinSize. This assumes the distance between leftBin and rightBin equals input.binSize. However, findRightBin returns the first bin where grid(idx) >= value, so the distance between inputGrid(leftBin) and inputGrid(rightBin) is actually input.binSize, making the formula correct. However, this should be verified through testing. Consider adding a check or assertion that rightBin == leftBin + 1 to ensure the bins are adjacent.

Might this be a hallucination? The reasoning of Copilot appears convoluted to me. There is no way that I can see that rightBin does not equal leftBin + 1, because of the definition auto leftBin = rightBin - 1 in the lines above the commented function. Any peculiarity might come from out-of-bounds values for either rightBin or leftBin, but this is handled already.

@XzzX XzzX merged commit 3d67ede into XzzX:main Jan 26, 2026
8 checks passed
@J-Hizzle
Copy link
Contributor Author

Sorry to open up this discussion again, but I'm insecure about the following: In interpolation.cpp:30-35, I implemented a Host assertion to check that the provided grid and the one from the created output MultiHistogram are the same. However, grid is most likely initialized in device space, as is done for instance in interpolation.test.cpp:48. Could this result in memory access errors? If so, why hasn't it given rise to any errors yet? Should I create a host space copy of the grid for the assertion?

@J-Hizzle
Copy link
Contributor Author

Another concern is that the assertion checks for equality of floats, which may lead to errors due to rounding. Should we maybe just parse a second inputGrid MultiHistogram which we then copy in order to ensure equal grid spacing rather than take the detour via creating a separate grid?

@XzzX
Copy link
Owner

XzzX commented Jan 27, 2026

Sorry to open up this discussion again, but I'm insecure about the following: In interpolation.cpp:30-35, I implemented a Host assertion to check that the provided grid and the one from the created output MultiHistogram are the same. However, grid is most likely initialized in device space, as is done for instance in interpolation.test.cpp:48. Could this result in memory access errors? If so, why hasn't it given rise to any errors yet? Should I create a host space copy of the grid for the assertion?

Yes. There is no CI job that runs on GPUs. You can use MRMD_DEVICE_ASSERT_EQUAL.

@XzzX
Copy link
Owner

XzzX commented Jan 27, 2026

Another concern is that the assertion checks for equality of floats, which may lead to errors due to rounding. Should we maybe just parse a second inputGrid MultiHistogram which we then copy in order to ensure equal grid spacing rather than take the detour via creating a separate grid?

Depends on your use case. You can use MRMD_HOST_ASSERT_FLOAT_EQUAL. This does an epsilon check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants