Repository: scipy/scipy
Pull request: MAINT: add `__bool__` method to spatial.transform.Rotation
<!-- 
Thanks for contributing a pull request! Please ensure that
your PR satisfies the checklist before submitting:
http://scipy.github.io/devdocs/dev/contributor/development_workflow.html#checklist-before-submitting-a-pr

Also, please name and describe your PR as you would write a
commit message:
http://scipy.github.io/devdocs/dev/contributor/development_workflow.html#writing-the-commit-message

Note that we are a team of volunteers; we appreciate your
patience during the review process.

Again, thanks for contributing!
-->

#### Reference issue
<!--Example: Closes gh-WXYZ.-->
Closes #16663 

#### What does this implement/fix?
<!--Please explain your changes.-->

Docstring from the new __bool__ method:

```python
        """Comply with Python convention for objects to be True.

        This method is necessary because the calling bool() on an object will
        fall back to the `Rotation.__len__()` if `Rotation.__bool__()` is not
        implemented and `Rotation.__len__()` is. If the Rotation is not a
        single its length is a positive integer wich is True as a boolean but
        if it is a single `Rotation.__len__()` raises a TypeError. A single
        Rotation object should also evaluate to True, hence this method so no
        fallback to `Rotation.__len__()` is necessary.
        """
```


Problem statement:
BUG: `bool(rotation)` leads to error
### Describe your issue.

Maybe it's undefined behavior, but calling `bool` on an instance of `scipy.spatial.transform.Rotation` yields an unclear error.

Perhaps any identity rotations could be Falsy, and anything else be truthy? (That's also why I tried `bool` on it)

### Reproducing Code Example

```python
from scipy.spatial.transform import Rotation
bool(Rotation.from_euler("x", 0))
```


### Error message

```shell
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "_rotation.pyx", line 563, in scipy.spatial.transform._rotation.Rotation.__len__
TypeError: Single rotation has no len().
```


### SciPy/NumPy/Python version information

1.8.1 1.22.2 sys.version_info(major=3, minor=9, micro=9, releaselevel='final', serial=0)

Hints from issue discussion:
Apologies for the tag churn; this is in fact a defect.

At least in Python 3.6 and scipy 1.4.1, `bool(some_rotation)` used to unconditionally return `True` without error. Now, it will return `True` in some cases but not others. Did something change in Python's `bool()` semantics since 3.6? Or Cython's code generation?

In any case, if we want to eliminate the `TypeError`, we should define `__bool__()` but unconditionally return `True` regardless of the contents.
It is not clear to me if and how the boolean transform of a Rotation is useful. If it is indeed not useful, throwing a TypeError with a clear error message from the `__bool__` method would be more useful.

Identity rotations could return False to indicate that, but this might be better put into a separate method check. e.g. in the form of `Rotation().yields_identity()` that then would return a boolean. Such an approach will lead to more readable code being written by scipy users.
Most objects in Python are simply truthy. Always returning `True` would be more Pythonic than a TypeError for the attempted conversion.
@Helveg You are right. https://docs.python.org/3/reference/expressions.html#boolean-operations
I wonder if `__len__` could return `1` instead of raising the TypeError.

https://github.com/scipy/scipy/blob/c1ce773461be108a703a7e4efd3de0d7c231b591/scipy/spatial/transform/_rotation.pyx#L548-L565
If `__len__` is implemented, `__bool__` uses it internally to assess the truthiness of an object. If it is so that Rotation can't be an instance with zero rotations I think it makes more sense to change the behaviour of `__len__` instead of implementing `__bool__`.
It seems at least one argument is necessary to instantiate a Rotation

```python
>>> len(Rotation([[0, 0, 0, 1], [0, 0, 0, 1]]))
2
>>> len(Rotation([0, 0, 0, 1]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "rotation.pyx", line 562, in scipy.spatial.transform.rotation.Rotation.__len__
TypeError: Single rotation has no len().
```

It is not clear to me why one single rotation should result in a TypeError, when in the case of multiple rotations length is the number of rotations. Wouldn't it be more consistent to make length equal 1 in the case of one rotation?
`[0, 0, 0, 1]` is distinct from `[[0, 0, 0, 1]]`.
Apparently:

```
>>> len(Rotation([[0, 0, 0, 1]]))
1
```

While the TypeError says "Single rotation has no len().", it is not clear to me why?
Basically, `Rotation` has a weird dual life, both as a single quaternion by itself and as the collection of quaternions. Those are distinct, and we have them behave distinctly. Just like `10.0` and `[10.0]` are distinct. `len(10.0)` raises a `TypeError` and does not return `1` even though both are just "single floats".

Commit message: DOC: accept review comment by shortening docstring

[skip github] [skip azp]

Co-authored-by: Fabian Egli <fabianegli@users.noreply.github.com>

Patch to review:
diff --git a/scipy/spatial/transform/_rotation.pyx b/scipy/spatial/transform/_rotation.pyx
index 860902892e3a..bff839d3006f 100644
--- a/scipy/spatial/transform/_rotation.pyx
+++ b/scipy/spatial/transform/_rotation.pyx
@@ -545,6 +545,14 @@ cdef class Rotation:
         """Whether this instance represents a single rotation."""
         return self._single
 
+    @staticmethod
+    def __bool__():
+        """Comply with Python convention for objects to be True.
+
+        Required because `Rotation.__len__()` is defined and not always truthy.
+        """
+        return True
+
     def __len__(self):
         """Number of rotations contained in this object.
 


Code review comments:
diff_hunk: @@ -545,6 +545,14 @@ cdef class Rotation:
         """Whether this instance represents a single rotation."""
         return self._single
 
+    @staticmethod
+    def __bool__():
path: scipy/spatial/transform/_rotation.pyx
text: @author:
Removing staticmethod decorator to hopefully solve the compile error.

```suggestion
    def __bool__(self):
```