Add Sparseness and Sparse Matrices - #177
Conversation
|
Note: I haven't updated the changelog yet because I anticipate that the final version of this feature might be different from the current implementation. |
Pull Request Test Coverage Report for Build 20489502116Details
💛 - Coveralls |
stephane-caron
left a comment
There was a problem hiding this comment.
Thank you for proposing this 👍 It makes sense to me that Pink would handle conversion upstream of qpsolvers (even though qpsolvers will do a similar conversion itself, issuing a warning for it).
My feedback below is mainly about keeping changes to a minimum. Maybe I missed something about the numpy-copy check, feel free to let me know if that's the case.
| from .tasks import Task | ||
|
|
||
|
|
||
| def _require_sparse_module(): |
There was a problem hiding this comment.
This function could return None rather than re-throw. It could simplify the logic below (see other comment).
| sparse_module = None | ||
| if use_sparse: | ||
| try: | ||
| sparse_module = _require_sparse_module() |
There was a problem hiding this comment.
If _require_sparse_module returns an optional module (i.e. module or None), then we could refactor this block into:
if use_sparse:
sparse_module = _require_sparse_module()
if sparse_module is not None:
P = ...
else: # sparse_module is None
warnings.warn("... (message from above)")| homogeneous. If it helps we can add a tangent-space scaling to damp the | ||
| floating base differently from joint angular velocities. | ||
| """ | ||
| if use_sparse and solver in _JAX_SOLVERS: |
There was a problem hiding this comment.
In both cases, use_sparse is set to False whenever the solver is a JAX solver. We could then simplify to:
if use_sparse and solver in _JAX_SOLVERS:
warnings.warn("Falling back to dense matrices as JAX solvers don't support sparsity.")
use_sparse = FalseWhat do you think?
Let me know if I missed anything. Otherwise I'd suggest we remove the NUMPY_SUPPORTS_COPY_KEYWORD handling entirely, as it will make the logic simpler.
|
|
||
| NUMPY_SUPPORTS_COPY_KEYWORD = _numpy_supports_copy_keyword() | ||
|
|
||
| SPARSE_SOLVERS = { |
There was a problem hiding this comment.
Those could be imported from qpsolvers.
|
|
||
| @unittest.skipIf( | ||
| sparse is None, | ||
| "SciPy is required for sparse tests. Install SciPy to run them.", |
There was a problem hiding this comment.
We can assume SciPy is installed in test environments.
| "NumPy copy keyword unsupported, skipping JAX-based solver." | ||
| ) | ||
| configuration, tasks, dt = self.get_jvrc_problem() | ||
| solve_ik(configuration, tasks, dt, solver=solver) |
There was a problem hiding this comment.
Can we keep this one as before, since the former test should still run?
If we want to introduce a second variant with use_sparse set to a non-default value, it should go to its own test. This way we grow the test fixture incrementally rather than revising it.
|
Thanks for the feedback Dr. Caron. I think I will make pretty significant changes to this PR to make it more minimal. I added the variable regarding NumPy since I was running into many errors locally without the check. However, what you pointed out makes it me think the codebase should be able to work with a much simpler approach, as you suggested. I made the commits in around September so I don't have the clearest memory about why I decided to use this roundabout method to address the incompatibility. I will revisit the codebase on my end to see if it is possible to remove the variable without running into issues. Thank you again for the detailed review! |
I added an optional
use_sparseargument inbuild_ik/solve_ikthat builds SciPy sparse QP matrices for sparse first solvers. I added warnings and fell back to dense matrices when JAX backends can't use them. I also check to see if SciPy is available and for NumPy'scopykeywords support so that the feature fails without erroring. I updated the IK tests to cover sparse assembly, solver compatibility (including JAX), and to skip w/o error when SciPy and NumPy behavior isn't available.If you have any questions or concerns regarding the current implementation, please let me know. I am happy to implement fixes or changes so that the implementation better suits pink's needs.