Skip to content

Conversation

@friedkeenan
Copy link
Member

@friedkeenan friedkeenan commented Apr 9, 2021

Changelog / Overview

  • Raise an error if the alpha argument is not between 0 and 1.
  • Raise an error if the :class:~.VMobject has no points.

Motivation

This will lead to less cryptic errors to users who use VMobject.point_from_proportion in the above ways. Currently it will just return None in those situations which makes it a bit tricky to track down the error. The previous implementation (before #1274) would have raised an error about it going beyond the number of its curves.

Explanation for Changes

Before doing any calculations, it's checked if alpha is between 0 and 1, and if not a ValueError is raised. Then .throw_error_if_no_points() is called which will raise an error with an appropriate message (though it's just a plain Exception).

I've also improved the method's docs and added accompanying tests.

Testing Status

Tests pass and docs look good locally.

Checklist

  • I have read the Contributing Guidelines
  • I have written a descriptive PR title (see top of PR template for examples)
  • I have written a changelog entry for the PR or deem it unnecessary
  • My new functions/classes either have a docstring or are private
  • My new functions/classes have tests added and (optional) examples in the docs
  • My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

  • The PR title is descriptive enough
  • The PR is labeled correctly
  • The changelog entry is completed if necessary
  • Newly added functions/classes either have a docstring or are private
  • Newly added functions/classes have tests added and (optional) examples in the docs
  • Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

@friedkeenan friedkeenan added the enhancement Additions and improvements in general label Apr 9, 2021
@jsonvillanueva jsonvillanueva changed the title Raise appropriate errors in :meth:VMobject.point_from_proportion Raise appropriate errors in :meth:~.VMobject.point_from_proportion Apr 10, 2021
Copy link
Member

@jsonvillanueva jsonvillanueva left a comment

Choose a reason for hiding this comment

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

Looks good, I've written some suggestions to improve UX on the site (back links to VMobject)... Unfortunately, OpenGLVMobject isn't indexed on our docs so the same stuff doesn't apply in opengl_vectorized_mobject.py.

I did want to briefly ask though, why not clamp the value between 0-1 as is done in other parts of the library (e.g. animations/clamping dt) instead of raising an error?

@friedkeenan
Copy link
Member Author

friedkeenan commented Apr 10, 2021

I'd say that if someone passes a proportion that's greater than 1 or less than 0, then either the code that passes the invalid proportion is bugged, or they understand the method wrong, both of which warrant an error. If they wish to have it clamped, then they can clamp the value themselves when passing it.

@friedkeenan
Copy link
Member Author

friedkeenan commented Apr 10, 2021

I improved the docs, and just used VMobject for the cairo version and OpenGLVMobject in the opengl version as I assume when the latter overtakes the former there'll just be a general find and replace

(also you merged master into my branch just as I was about to push so that's why there's an extra commit)

@friedkeenan
Copy link
Member Author

The RTD build failing is not my fault afaict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Additions and improvements in general

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants