Added OMPLRangedPlanner class #353
Conversation
This class adds logic for setting a 'range' parameter such that an integer number of state validity checks will occur per extension. It improves on the previous RRTConnect class in two ways: 1. Accepts an optional 'algorithm' parameter, e.g. for PR_RRTConnect. 2. Allows for more than one state validity checks per extension.
| """ | ||
| def __init__(self, algorithm='RRTConnect', robot_checker_factory=None, | ||
| multiplier=1): | ||
| if multiplier < 1 or not isinstance(multiplier, int): |
There was a problem hiding this comment.
It's a bit weird to order these checks this way; I would reverse it for clarity.
There was a problem hiding this comment.
Also, doesn't this mean that a __lt__ or __cmp__ override on multiplier could actually succeed here even if it is not an int?
There was a problem hiding this comment.
Yeah that's what I was thinking (though the second case would still catch those, it's still a bit confusing).
There was a problem hiding this comment.
Would it? I thought it would short-circuit the or... oh i see, that's ok.
There was a problem hiding this comment.
Actually, now I'm wondering if we should just not do the isinstance check. If someone wants to pass a dynamically evaluated property or something, I don't see a great reason to shut them down here if it duck-types properly.
There was a problem hiding this comment.
Maybe replace it with if not multiplier >= 1: to allow such types, still catch the typical non-positive case (which might silently yield borked results), and correctly error if NaN is passed?
|
|
||
| # Duplicate the logic in or_ompl to convert the DOF resolutions to | ||
| # a fraction of the longest extent of the state space. | ||
| maximum_extent = numpy.max(dof_ranges) |
There was a problem hiding this comment.
Unfortunately, that's not what the maximum extent is ... you should use numpy.linalg.norm instead of numpy.max.
|
@cdellin: I addressed your comments. Could you take another look? |
|
Looks great! |
This class adds logic for setting a 'range' parameter such that an
integer number of state validity checks will occur per extension. It
improves on the previous RRTConnect class in two ways:
This also fixes #350.