Skip to content

fix: add exception in Quaternion.average#35

Open
MarcoMiretti wants to merge 1 commit intosatellogic:masterfrom
MarcoMiretti:fix-add-exception-when-averaging-empty-list
Open

fix: add exception in Quaternion.average#35
MarcoMiretti wants to merge 1 commit intosatellogic:masterfrom
MarcoMiretti:fix-add-exception-when-averaging-empty-list

Conversation

@MarcoMiretti
Copy link
Copy Markdown

When trying to average an empty list of quaternions, rise a
QuaternionError, explaining that it takes at least one quaternion to
calculate the average.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 7, 2020

Coverage Status

Coverage increased (+0.01%) to 98.479% when pulling 6a6fae3 on MarcoMiretti:fix-add-exception-when-averaging-empty-list into 649d47b on satellogic:master.

Copy link
Copy Markdown
Collaborator

@matiasg matiasg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Just one comment on the test, but +1 from me.

self.fail('Expected QuaternionError exception to be raised')
except QuaternionError:
pass
except np.linalg.LinAlgError:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this exception in particular?
Could it be just except Exception?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i guess that's better.

Just to explain why i met this particular exception:
np.linalg.LinAlgError is what happens when we try to np.linalg.eigh(matrix) for a matrix of rank less than two.

In the case of an empty quaternion list gives us:

b = np.array([])
weights = np.array([])

then:

m = 0.0

So we end up calling:

cls._first_eigenvector(0.0)

which leads to:

np.linalg.eigh(0.0)
--> np.linalg.LinAlgError

In this test, except Exception is much better, since for example, _first_eigenvector could change in the future, checking the rank of its input and raising exceptions if they aren't met. That would break this test unnecessarily.

When trying to average an empty list of quaternions, rise a
QuaternionError, explaining that it takes at least one quaternion to
calculate the average.
@MarcoMiretti MarcoMiretti force-pushed the fix-add-exception-when-averaging-empty-list branch from e219864 to 6a6fae3 Compare September 7, 2020 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants