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 #347

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Fix #347

merged 1 commit into from
Jun 11, 2024

Conversation

AinurZiga
Copy link
Contributor

@AinurZiga AinurZiga commented Mar 4, 2024

Trying Sionna-RT for my research, I noticed strange behavior with wedges/edges and, particularly, a method for swapping edge extremities to orient identical edges in the same way. The problem was, as I understood that some coordinates (for example, r0 and r1) might coincide, but due to numerical issues, one might be larger than another (r0 > r1 yields True).

I changed the code, so comparison for nearly equal values starts before checking what number is larger.

My 1st commit contains an example where I have observed the problem. All edges have equal radii in spherical coordinates - due to numerical issues, r0 > r1 may give True.
My 2nd commit contains changes to the method for swapping edge extremities.

The code snippet which I used to reproduce the problem.

scene = load_scene(sionna.rt.scene.sphere)
num_edges = tf.reduce_sum(tf.cast(scene._solver_paths._is_edge, tf.int32))
num_wedges = scene._solver_paths._wedges_origin.shape[0] - num_edges
print(num_edges, num_wedges)

Expected output/output after the fix: 0 120
Output before the fix: 44 98

Signed-off by: Ainur Ziganshin [email protected]

Checklist

  • Detailed description
  • Added references to issues and discussions
  • Added / modified documentation as needed
  • Added / modified unit tests as needed
  • Passes all tests
  • Lint the code
  • Performed a self review
  • Ensure you Signed-off the commits. Required to accept contributions!
  • Co-authored with someone? Add Co-authored-by: user@domain and ensure they signed off their commits too.

@faycalaa
Copy link
Collaborator

Hi,
Thank you for this fix @AinurZiga!

Could you please delete the commit that adds the sphere scene and only keep the one implementing the fix?

Comparison to find the larger coordinate starts with
checking if these coordinates are close to avoid numerical
problems with float values.

Signed-off by: Ainur Ziganshin
[email protected]
@AinurZiga
Copy link
Contributor Author

Hi, Thank you for this fix @AinurZiga!

Could you please delete the commit that adds the sphere scene and only keep the one implementing the fix?

Hi Fayçal,
I removed the unnecessary commit.

@faycalaa
Copy link
Collaborator

Thank you!

@jhoydis jhoydis merged commit 1438bbf into NVlabs:main Jun 11, 2024
4 checks passed
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.

3 participants