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 division by zero when running the tutorial experiment #109

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

d-led
Copy link
Contributor

@d-led d-led commented Dec 11, 2024

Problem

when running

% python benchmarks/run.py -e randrot_noise_10distinctobj_surf_agent
# or
% python benchmarks/run_parallel.py -e randrot_noise_10distinctobj_surf_agent

the following run-time errors can be observed:

.../tbp/monty/frameworks/models/motor_policies.py:1259: RuntimeWarning: divide by zero encountered in scalar divide
  return -np.degrees(np.arctan(x / z))
.../numpy/core/fromnumeric.py:3464: RuntimeWarning: Mean of empty slice.
  return _methods._mean(a, axis=axis, dtype=dtype,
.../numpy/core/_methods.py:192: RuntimeWarning: invalid value encountered in scalar divide
  ret = ret.dtype.type(ret / rcount)

as these might affect numeric results, an analysis is necessary. This has implications for published benchmarks that were affected.

Analysis

>>> import numpy as np
>>> x=-1
>>> z=0
>>> -np.degrees(np.arctan(x / z))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ZeroDivisionError: division by zero

interestingly in the shell context this is a run-time error. Is this somehow (mis-)configured to be ignored?

Fix

Fix orienting_angle_from_normal to not be affected by division by zero

Result

> python benchmarks/run.py -e randrot_noise_10distinctobj_surf_agent
...
 overall/percent_correct 100

@d-led d-led force-pushed the fix-division-by-zero branch 2 times, most recently from 046d572 to 19b3ff5 Compare December 11, 2024 17:03
@d-led d-led changed the title Fix 3 RuntimeWarnings when running the tutorial experiment Draft: Fix 3 RuntimeWarnings when running the tutorial experiment Dec 11, 2024
@d-led d-led changed the title Draft: Fix 3 RuntimeWarnings when running the tutorial experiment Draft: Fix division by zero when running the tutorial experiment Dec 11, 2024
@tristanls tristanls added the triaged This issue or pull request was triaged label Dec 11, 2024
@tristanls
Copy link
Contributor

@d-led d-led changed the title Draft: Fix division by zero when running the tutorial experiment Fix division by zero when running the tutorial experiment Dec 11, 2024
@vkakerbeck
Copy link
Contributor

Wow, thanks for taking a look at this and finding this fix!
We will validate this against some of our longer benchmark experiments.

  • If it all looks good and it indeed improves the benchmark results we will open a corresponding PR to update the results table (since currently we don't have a way for external people to run on comparable infrastructure to have the timing column make sense).
  • If it doesn't affect performance it will be a nice fix anyways and we can just merge without updating the results table.
  • If it impacts performance negatively we'll let you know.

(If you just want to get an impression yourself of how accuracy is affected on more challenging experiments you could also try running one of these benchmarks yourself and just ignore the runtimes when comparing https://thousandbrainsproject.readme.io/docs/benchmark-experiments#results-1)

@d-led d-led force-pushed the fix-division-by-zero branch from 19b3ff5 to 94c001d Compare December 11, 2024 19:31
@d-led
Copy link
Contributor Author

d-led commented Dec 11, 2024

> python benchmarks/run.py -e base_77obj_dist_agent
...
overall/percent_correct 95.671

vs 93.51% in the table. Eager to see complete results

@d-led d-led force-pushed the fix-division-by-zero branch from 94c001d to b197199 Compare December 11, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged This issue or pull request was triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants