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

Use of numpy.isclose method #63

Closed
jeking04 opened this issue Mar 6, 2015 · 6 comments
Closed

Use of numpy.isclose method #63

jeking04 opened this issue Mar 6, 2015 · 6 comments
Labels

Comments

@jeking04
Copy link
Contributor

jeking04 commented Mar 6, 2015

Currently, to use the numpy.isclose method we need to install numpy from pip rather than apt. This is a bit cumbersome. Can we remove this dependency?

@cdellin
Copy link
Contributor

cdellin commented Mar 6, 2015

Since the implementation [0] is only 23 lines, maybe we can just maintain our own version and hot-patch it in when we import numpy if it isn't there?

[0] https://github.com/numpy/numpy/blob/v1.9.1/numpy/core/numeric.py#L2238

@psigen
Copy link
Member

psigen commented Mar 6, 2015

Is this an issue in 14.04 as well or only in 12.04?

On Fri, Mar 6, 2015 at 3:06 PM, cdellin notifications@github.com wrote:

Since the implementation [0] is only 23 lines, maybe we can just maintain
our own version and hot-patch it in when we import numpy if it isn't there?

[0] https://github.com/numpy/numpy/blob/v1.9.1/numpy/core/numeric.py#L2238


Reply to this email directly or view it on GitHub
#63 (comment)
.

@mkoval
Copy link
Member

mkoval commented Mar 6, 2015

I believe the version of NumPy shipped with 14.04 has is_close.

More importantly, I'm not sure if we want to use is_close in this context
anyway. Aren't the default tolerances much tigher than what we care about?

On Fri, Mar 6, 2015 at 3:08 PM, Pras Velagapudi notifications@github.com
wrote:

Is this an issue in 14.04 as well or only in 12.04?

On Fri, Mar 6, 2015 at 3:06 PM, cdellin notifications@github.com wrote:

Since the implementation [0] is only 23 lines, maybe we can just maintain
our own version and hot-patch it in when we import numpy if it isn't
there?

[0]
https://github.com/numpy/numpy/blob/v1.9.1/numpy/core/numeric.py#L2238


Reply to this email directly or view it on GitHub
<
https://github.com/personalrobotics/prpy/issues/63#issuecomment-77626241>

.


Reply to this email directly or view it on GitHub
#63 (comment)
.

@siddhss5
Copy link

siddhss5 commented Mar 9, 2015

OK, how is one supposed to programmatically decide if a configuration is close enough to another?
Is it with: all(abs(a-b) < robot.GetActiveDOFResolutions())?

If so, or if it's something slightly different, it'd be nice if that were in prpy.util, no?

@mkoval
Copy link
Member

mkoval commented Mar 9, 2015

I see two places where numpy.isclose is used:

  1. In ComputeJointVelocityFromTwist:
dq_bounds = [(0, max) if (numpy.isclose(q_curr[i], q_min[i])) else
             (min, 0) if (numpy.isclose(q_curr[i], q_max[i])) else
             (min, max) for i, (min, max) in enumerate(bounds)]

Here, I think it makes sense to use GetActiveDOFResolution like you suggested.

  1. In PlanWorkspacePath:
while not numpy.isclose(t, traj.GetDuration()):

Here, I think this should be replaced with t < traj.GetDuration().

@siddhss5 Thoughts?

@psigen
Copy link
Member

psigen commented Apr 9, 2015

@siddhss5, @mkoval: Has this been addressed/fixed already?

I think @mkoval's suggestions make sense, and while I am fine with using numpy.isclose() in general, I don't think it's necessary in our codebase right now since there aren't too many cases where an exact comparison is actually what we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants