Skip to content

Even more typing#200

Open
TheBB wants to merge 23 commits intoSINTEF:masterfrom
TheBB:more-more-typing
Open

Even more typing#200
TheBB wants to merge 23 commits intoSINTEF:masterfrom
TheBB:more-more-typing

Conversation

@TheBB
Copy link
Member

@TheBB TheBB commented Jan 2, 2026

This PR changes some of the type aliases.

  • Let's stop using Numpy's ArrayLike. It's both too general and not general enough: it includes some things that aren't relevant to us, and it excludes (or at least, Mypy seems to be unable to figure out) a few things that we need.
  • Same for Scalar: it's too general
  • Add some variation to our types: Point, Points, ControlPoints, Knots. Many of these overlap and are similar, but we can use them to signal intent to the user, which is especially useful for all the functions with parameters named x, t, u, etc.

I'll go through the already-typed files to make some changes.

@VikingScientist
Copy link
Member

The term "Knots" here is a bit misleading. It refers to a very specific term in the knot vector which the NURBS book denotes as breakpoints. It is defined on the parameter space, but the parametric space is far more general and includes a lot more points than just the knots from the knot vector.

As I understand your proposed change then it is to make the distinction between parametric values and geometric values. i.e. for a spline surface
$$x(u) = \sum_i N_i(u) x_i$$
then x can have multiple components and is defined in the physical space (typehint Point) and u can have multiple components and is defined in the parametric space (typehint Knots). I would object to this notation as a more accurate typehints would be ParametricPoint and GeometricPoint. I don't really like this solution either as it is too verbose. But simply calling it Knot is not good enough either as Knot is to ParametricPoint what ControlPoint is to GeometricPoint.

I do think it is a good idea to have ControlPoints specified as their own typehints since they both have a particular interpretation from at mathematical point of view, and have a special structure (up to 4D tensors) from an implementation point of view.

def evaluate(
self,
t: ArrayLike | Scalar,
t: Knots | Scalar,
Copy link
Member

@VikingScientist VikingScientist Jan 3, 2026

Choose a reason for hiding this comment

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

This is one of the places where I object to referring to parametric evaluation points t as knots. They simply are not knots and it is more misleading to call them as such.

@TheBB TheBB force-pushed the more-more-typing branch from 287c048 to 333ed7a Compare January 3, 2026 22:49
@TheBB
Copy link
Member Author

TheBB commented Jan 3, 2026

26 errors left!

@TheBB
Copy link
Member Author

TheBB commented Jan 4, 2026

Screenshot from 2026-01-04 11-03-29

@TheBB
Copy link
Member Author

TheBB commented Jan 4, 2026

I don't really like this solution either as it is too verbose. But simply calling it Knot is not good enough either as Knot is to ParametricPoint what ControlPoint is to GeometricPoint.

Well, what would you prefer then? Vector? FloatVector?

We have the following kinds of (long) one-dimensional vectors floating around:

  • Knot vectors
  • Evaluation parameters

We also have Point, which is a short one-dimensional vector (generally three elements or fewer), which I prefer to have a different name, even if they are conceptually basically the same as the above.

And we have these two- or multi-dimensional array types:

  • Points
  • Control points

I understand that you want a better name for the first group?

TheBB added 3 commits January 4, 2026 16:14
This adds some mypy ignore rules to make type checking work even when certain libraries are not installed.
@TheBB TheBB force-pushed the more-more-typing branch from c8b019e to 81ac9cf Compare January 4, 2026 16:35
@TheBB TheBB force-pushed the more-more-typing branch from 81ac9cf to e87ea66 Compare January 4, 2026 16:44
@TheBB TheBB marked this pull request as ready for review January 4, 2026 20:02
@TheBB TheBB linked an issue Jan 5, 2026 that may be closed by this pull request
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.

Implement the Point typehint

2 participants