Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check Object documentation for Glyph(?) #743

Open
knutnergaard opened this issue Aug 19, 2024 · 16 comments
Open

Check Object documentation for Glyph(?) #743

knutnergaard opened this issue Aug 19, 2024 · 16 comments

Comments

@knutnergaard
Copy link
Contributor

knutnergaard commented Aug 19, 2024

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 knutnergaard changed the title Check Object documentation for Layer(?) Check Object documentation for Glyph(?) Aug 19, 2024
@benkiel
Copy link
Member

benkiel commented Aug 21, 2024

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

@knutnergaard
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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?

@LettError
Copy link
Member

What do you mean with normalising the factor?

@knutnergaard
Copy link
Contributor Author

knutnergaard commented Sep 18, 2024

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?

@LettError
Copy link
Member

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.

@knutnergaard
Copy link
Contributor Author

knutnergaard commented Sep 18, 2024

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.

@typesupply
Copy link
Member

typesupply commented Sep 18, 2024

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.

@knutnergaard
Copy link
Contributor Author

knutnergaard commented Sep 18, 2024

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.

@typesupply
Copy link
Member

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.

@knutnergaard
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@benkiel
Copy link
Member

benkiel commented Sep 18, 2024

Number feels right here

@knutnergaard
Copy link
Contributor Author

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.

@benkiel
Copy link
Member

benkiel commented Sep 18, 2024

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.

@knutnergaard
Copy link
Contributor Author

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:
#741 (comment)

Thanks!

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

No branches or pull requests

4 participants