Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Spatial Wrapped invalid env/radii detection #1182

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ptheywood
Copy link
Member

  • Reproduce python-native edge case in c++ test suite
  • Expand edge case tests coverage to exposes a broader range of floating point issues
  • Fix tests
  • Optional: Migrate float logic to the host, making in-device checks a single global read.

Closes #1177

@ptheywood
Copy link
Member Author

The following appears to work for values tested so far. Not the cheapest method so worth only doing this once on the host and storing if wrapped is available for a given spatial message list on the device, rather than doing this per call to the wrapped iterator on the device

template <typename T>
bool approxExactlyDivisible(T a, T b) {
    // Scale machine epsilon by the magnitude of the larger value
    T scaledEpsilon = std::max(std::abs(a), std::abs(b)) * std::numeric_limits<T>::epsilon();
    // Compute the remainder
    T v = std::fmod(a, b);
    // approx equal if the remainder is within scaledEpsilon of 0 or b (fmod(1, 0.05f) returns ~0.05f)
    return v <= scaledEpsilon || v > b - scaledEpsilon;
}

Uses scaled machine epsilon to compare two floats to detertmine if spatial env width is a multiple of the radius.

Only in a test suite so far, with some tests showing it seems to work for previously bad cases.

Todo: Move into the spatial messaging classes, compute on the host, copy if wrapped is available to device, query at runtime in seatbelts mode.
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.

Wrapped MsgSpatial interaction radius factor bug
1 participant