fontParts icon indicating copy to clipboard operation
fontParts copied to clipboard

Check Object documentation for Glyph(?)

Open knutnergaard opened this issue 1 year ago • 24 comments

Even though there is no open issue with regard to this, I'm assuming glyph.py should be revised with type annotations and updated docstrings, similarly to other modules in the pipeline.

If so, there is an issue with the type hinting of the pen-related methods: what should their annotated pen type actually be?

One option, which is in line with my revisions in font.py and layer.py, is to use the appropriate abstract base classes from FontTools (like AbstractPen and AbstractPointPen. This is straightforward and allows direct cross reference to their documentation, but mandates a dependency on FontTools. Another, more flexible, but complicated option is to define specific typing protocols for pen objects, which only restricts the environment objects in terms of how certain methods should be defined.

I'm not sure which of these is the best for FontParts, or how a protocol for e.g., Pen and PointPen should be defined. What say you, @benkiel, @typesupply?

knutnergaard avatar Aug 19 '24 12:08 knutnergaard

The dependency on fontTools AbstractPen and AbstractPointPen is ok — we already have that dependency, and fontTools is the standard now for the pen protocol

benkiel avatar Aug 21 '24 14:08 benkiel

@benkiel, does it make sense to mandate subclassing of FontMath objects on similar grounds (e.g, with regard to the BaseGlyph._toMathGlyph and BaseGlyph._fromMathGlyph methods?

Also, are the BaseObject classes and the mixin classes intended to be part of the documentation? There are a few cases where it would make sense to reference them in other classes.

knutnergaard avatar Sep 01 '24 11:09 knutnergaard

@benkiel, @typesupply, there seems to be a discrepancy between the base level interpolation function and the higher level interpolation methods.

base.interpolate would seem to indicate the following annotation, with (to my understanding) a being the minimum value, b being the maximum value and v being the interpolation factor:

IntFloatType = Union[int, float]

def interpolate(a: IntFloatType, b: IntFloatType, v: IntFloatType) -> IntFloatType:
    return a + (b - a) * v

BaseGlyph.interpolate OTOH passes in the following types to the above function:

a: MathGlyph
b: MathGlyph
v: Union[IntFloatType, Tuple[IntFloatType, IntFloatType]]

Even if base.interpolate were to accept the MathGlyph objects as a numeric value, the factor type needs to be normalised to not raise an error here, or am I missing something?

knutnergaard avatar Sep 18 '24 10:09 knutnergaard

What do you mean with normalising the factor?

LettError avatar Sep 18 '24 10:09 LettError

What do you mean with normalising the factor?

Since factor can be either int, float or tuple, it will have to be normalised as an int or float before being passed to base.interpolate, no?

knutnergaard avatar Sep 18 '24 11:09 knutnergaard

Not sure I understand why that would be useful?

  • the factor is generally 0 < f < 1, so floats are kind of necessary.
  • extrapolations are possible, the factor can also be < 0 and >1.
  • A tuple factor is understood to be for anisotropic interpolation. The first value is the horizontal factor, the second value is the vertical factor.

LettError avatar Sep 18 '24 11:09 LettError

Not sure I understand why that would be useful?

  • the factor is generally 0 < f < 1, so floats are kind of necessary.
  • extrapolations are possible, the factor can also be < 0 and >1.
  • A tuple factor is understood to be for anisotropic interpolation. The first value is the horizontal factor, the second value is the vertical factor.

OK, I'm clearly missing something then. My objection was purely syntactical. There is no way that I can see for base.interpolation to discern a tuple factor from a float factor, and a + (b - a) * (vx, vy) is not valid syntax as far as I know.

knutnergaard avatar Sep 18 '24 11:09 knutnergaard

I might be able to help clarify...

base.interpolate takes any object that emulates a numeric type for a and b. This is done by implementing __add__, __sub__, __mul__, __div__, etc. in the objects. We've built objects (glyphs, contours, colors, more) like this all over the place that get sent through that interpolate function. Some of the objects in fontMath take a tuple in multiplication and division operations to enable anisotropic interpolation. For example, look at mul and mulPt here.

So, a and b are any object that emulates a numeric type and f is a numeric type or a tuple of two numeric types. Does the type annotation have a way of specifying "any object that emulates a numeric type"? This interpolate function and the things that are passed through it go way back* to the pre-typing days, but the functionality is extraordinarily useful.

I hope that makes sense. My brain is not fully booted up yet this morning. Please let me know if you need more info.

*Full credit because this isn't documented anywhere: @LettError came up with the idea that glyphs could behave like numbers and that would allow us to use mathematical equations to process outlines very easily. It is one of the most brilliant tools things we have to work with. I'm still in awe of it and what it has enabled.

typesupply avatar Sep 18 '24 11:09 typesupply

I might be able to help clarify...

base.interpolate takes any object that emulates a numeric type for a and b. This is done by implementing __add__, __sub__, __mul__, __div__, etc. in the objects. We've built objects (glyphs, contours, colors, more) like this all over the place that get sent through that interpolate function. Some of the objects in fontMath take a tuple in multiplication and division operations to enable anisotropic interpolation. For example, look at mul and mulPt here.

So, a and b are any object that emulates a numeric type and f is a numeric type or a tuple of two numeric types. Does the type annotation have a way of specifying "any object that emulates a numeric type"? This interpolate function and the things that are passed through it go way back* to the pre-typing days, but the functionality is extraordinarily useful.

I hope that makes sense. My brain is not fully booted up yet this morning. Please let me know if you need more info.

*Full credit because this isn't documented anywhere: @LettError came up with the idea that glyphs could behave like numbers and that would allow us to use mathematical equations to process outlines very easily. It is one of the most brilliant tools things we have to work with. I'm still in awe of it and what it has enabled.

As I mentioned earlier, I accept that a and b may be "any object that emulates a numeric type". However, I still don't get how factor may be either a numeric singleton or a tuple without the need for any further normalisation or logic. E.g., interpolate(1, 2, 3) is fine, but interpolate(1, 2, (3,4)) throws an error.

I realise that this code goes way back, and, as such, must be thoroughly tested. I just don't see how it works. :)

BTW, I don't know of a type that denotes emulated numeric types, but I guess those objects would usually inherit from numeric types? Otherwise, Any is probably most appropriate, even if that bypasses the static type checking completely.

knutnergaard avatar Sep 18 '24 12:09 knutnergaard

However, I still don't get how factor may be either a numeric singleton or a tuple without the need for any further normalisation or logic. E.g., interpolate(1, 2, 3) is fine, but interpolate(1, 2, (3,4)) throws an error.

Ah. I forgot to explain this. The normalization happens inside of the fontMath objects. For example, in MathGlyph: if a singleton comes in as the factor, it is converted to a tuple.

Then:

So, you are correct that interpolate(1, 2, (3, 4)) would raise an error. But, interpolate(glyph1, glyph2, (3, 4)) where the glyphs are MathGlyph would work.

typesupply avatar Sep 18 '24 13:09 typesupply

However, I still don't get how factor may be either a numeric singleton or a tuple without the need for any further normalisation or logic. E.g., interpolate(1, 2, 3) is fine, but interpolate(1, 2, (3,4)) throws an error.

Ah. I forgot to explain this. The normalization happens inside of the fontMath objects. For example, in MathGlyph: if a singleton comes in as the factor, it is converted to a tuple.

Then:

So, you are correct that interpolate(1, 2, (3, 4)) would raise an error. But, interpolate(glyph1, glyph2, (3, 4)) where the glyphs are MathGlyph would work.

Thanks! I was confused when I couldn't find the normalizers.normalizeInterpolationFactor in the same place as the call to base.interpolate within BaseGlyph._interpolate, but of course, the normalisation was done in the public method.

knutnergaard avatar Sep 18 '24 13:09 knutnergaard

Does the type annotation have a way of specifying "any object that emulates a numeric type"? This interpolate function and the things that are passed through it go way back* to the pre-typing days, but the functionality is extraordinarily useful.

It seems that the builtin numbers module provides an abstract base class Number which may be used for annotation as well as real time type checking of number-like objects. It's accepted by Mypy, and so seems like the best choice for these kinds of cases.

knutnergaard avatar Sep 18 '24 14:09 knutnergaard

Number feels right here

benkiel avatar Sep 18 '24 15:09 benkiel

Number feels right here

@benkiel, great! BTW, do you have any view on this?

@benkiel, does it make sense to mandate subclassing of FontMath objects on similar grounds (e.g, with regard to the BaseGlyph._toMathGlyph and BaseGlyph._fromMathGlyph methods?

Also, are the BaseObject classes and the mixin classes intended to be part of the documentation? There are a few cases where it would make sense to reference them in other classes.

knutnergaard avatar Sep 18 '24 15:09 knutnergaard

I think it is fine to mandate subclassing of FontMath objects there, as they depend on that.

The BaseObject classes aren't intended to be part of the documentation, can see where some mixin classes would be useful.

As a general philosophy, the documentation generated on the website is intended for folks writing scripts. Documentation for folks implementing fontParts can be in code, only surfacing to the website where it makes sense to help scripters, if that makes sense. Tricky with the two audiences, I know.

benkiel avatar Sep 18 '24 15:09 benkiel

I think it is fine to mandate subclassing of FontMath objects there, as they depend on that.

The BaseObject classes aren't intended to be part of the documentation, can see where some mixin classes would be useful.

As a general philosophy, the documentation generated on the website is intended for folks writing scripts. Documentation for folks implementing fontParts can be in code, only surfacing to the website where it makes sense to help scripters, if that makes sense. Tricky with the two audiences, I know.

@benkiel, I see. I guess it doesn't hurt to reference them in other classes using Sphinx syntax anyway. Even if the links won't work in the generated documentation, these references do clarify where to find the code they're pointing to.

While I have your attention, would you mind answering this post as well: https://github.com/robotools/fontParts/issues/741#issuecomment-2316874443

Thanks!

knutnergaard avatar Sep 18 '24 19:09 knutnergaard

However, I still don't get how factor may be either a numeric singleton or a tuple without the need for any further normalisation or logic. E.g., interpolate(1, 2, 3) is fine, but interpolate(1, 2, (3,4)) throws an error.

Ah. I forgot to explain this. The normalization happens inside of the fontMath objects. For example, in MathGlyph: if a singleton comes in as the factor, it is converted to a tuple.

Then:

So, you are correct that interpolate(1, 2, (3, 4)) would raise an error. But, interpolate(glyph1, glyph2, (3, 4)) where the glyphs are MathGlyph would work.

BTW, @typesupply, since the normalisation in this case happens in the public method and not the private one, am I correct in assuming that the private method should not accept a tuple factor?

knutnergaard avatar Sep 28 '24 11:09 knutnergaard

@knutnergaard for base.glyph._interpolate factor must be a tuple, that's what the normalizer does in the public method.

I know the issue is still the oddity of base.interpolate working with factor being a single value or a tuple; if the concern is that there is no checking there, I'm not sure how to do it other than a try/except in base.interpolate that throws a error (something like: factor must be a single value) if the two things being interpolated don't support (ie., are not mathGlyphs) the tuple factor.

benkiel avatar Sep 28 '24 17:09 benkiel

@benkiel Problem is, the normalisation of factor happens in BaseGlyph.interpolate before the call to BaseGlyph._interpolate, so BaseGlyph._interpolate factor never receives a tuple.

In my mind, if factor in the private method also should be able to receive a tuple, normalizeInterpolationFactor should be moved to the private method.

knutnergaard avatar Sep 28 '24 20:09 knutnergaard

@knutnergaard sorry, I'm confused here: afaik, normalizeInterpolationFactor returns of tuple: https://github.com/robotools/fontParts/blob/f72b3c15b8502e899980e6724ce7ed3c496ba249/Lib/fontParts/base/normalizers.py#L975 Am I missing something?

benkiel avatar Sep 30 '24 18:09 benkiel

@knutnergaard sorry, I'm confused here: afaik, normalizeInterpolationFactor returns of tuple:

https://github.com/robotools/fontParts/blob/f72b3c15b8502e899980e6724ce7ed3c496ba249/Lib/fontParts/base/normalizers.py#L975

Am I missing something?

@benkiel Me too! You're absolutely right that the normaliser returns a tuple. I see that now. But how can that be when base.interpolate cannot receive a tuple, as discussed above?

I think it's me that's missing something. Sorry if this should be obvious.

knutnergaard avatar Sep 30 '24 19:09 knutnergaard

@knutnergaard it's not you, it's confusing!

What is happening is this:

mathGlyph has a __mult__ function that uses tuples (https://github.com/robotools/fontMath/blob/f8a68f9f867357c039a42f15dae781fd34fdb838/Lib/fontMath/mathGlyph.py#L206) so when you do a base.interpolate with a factor that is a tuple, it's valid.

Normal numbers (int, float) clearly don't have a __mult__ that deals with floats, so you get an error.

An idea (maybe dumb): base.interpolate does a try/except, if it errors due to not being able to multiply by a tupple, it returns an error "Factor must be a int or float. This may not be any better than the error you'd get anyway, but a thought.

benkiel avatar Oct 01 '24 16:10 benkiel

@knutnergaard it's not you, it's confusing!

What is happening is this:

mathGlyph has a __mult__ function that uses tuples (https://github.com/robotools/fontMath/blob/f8a68f9f867357c039a42f15dae781fd34fdb838/Lib/fontMath/mathGlyph.py#L206) so when you do a base.interpolate with a factor that is a tuple, it's valid.

Normal numbers (int, float) clearly don't have a __mult__ that deals with floats, so you get an error.

An idea (maybe dumb): base.interpolate does a try/except, if it errors due to not being able to multiply by a tupple, it returns an error "Factor must be a int or float. This may not be any better than the error you'd get anyway, but a thought.

Thanks for the clarification, @benkiel. Your suggestion to add a try/except to base.interpolate is a good one, and so I've added it.

Also, when including tuple in the annotation for factor, numbers.Number will no longer work. I've added a custom protocol Interpolatable to base.annotations that implements __add__, __sub__ and __mul__ accompanied by the variable InterpolatableType that binds it.

knutnergaard avatar Oct 02 '24 12:10 knutnergaard

Sorry for being slow to respond. These solutions sound good. Thanks for working through this.

This is one of those "If we were making it today, we'd do it differently." situations. Some of this code is well over 20 years old. I'm amazed that it's still working. 🎉 for Python's stability.

typesupply avatar Oct 03 '24 13:10 typesupply

@knutnergaard close this as done in v1?

benkiel avatar Nov 04 '24 20:11 benkiel