Skip to content

Conversation

@rocky
Copy link
Member

@rocky rocky commented Feb 3, 2026

  • Add Box type annotations to render routines
  • Remove "_" from non-private classes. Go over render docstrings.
  • Fix type errors found by mypy:
    • Initialize BoxExpression.boxes.
    • Initialize GraphicsBox.boxwidth.
    • Initialize GraphicsBox.boxheight

Also: remove "_" from non-private classes. Go over render docstrings.
@rocky
Copy link
Member Author

rocky commented Feb 3, 2026

@mmatera: you asked for changes in #1671 to be split out.

Here, we just go over type annotations. This aspect was, in fact, a larger problem than just MathML. But it should come first because, in order have a more informed discussion of MathML, we need a better foundation.

The changes here seem larger than what's really going on: renaming "self" to "box" and adding specific type definitions. (And in doing so, already I am seeing a type failure.)

@mmatera
Copy link
Contributor

mmatera commented Feb 3, 2026

I am seing now some type errors in master too, with the latest version of mypy (1.19.1). Some of the errors here should be fixed by adding an assertion / condition to narrow the type of the argument. But I guess that in all the render routines, the type of the argument box should be BoxElementMixin, and then narrow them inside each function.

@mmatera
Copy link
Contributor

mmatera commented Feb 3, 2026

BTW, thanks for the work of splitting this in parts.



class _GraphicsElementBox(BoxExpression, ABC):
class GraphicsElementBox(BoxExpression, ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the underscore was to avoid considering this class as a Builtin to be shown in the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

(In WMA this builtin does not exist)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the underscore was to avoid considering this class as a Builtin to be shown in the documentation.

The entire module is marked no-doc. So this should no longer be a reason for having an underscore. In Python, an underscore typically indicates a private class, and this is not private. If it had an underscore previously, that was the wrong way to solve the problem. But at any rate this problem does not exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

(In WMA this builtin does not exist)

I don't understand what this is getting at. We have many classes in Mathics3 and some built-in function classes that do not exist in WMA.

Copy link
Contributor

@mmatera mmatera Feb 3, 2026

Choose a reason for hiding this comment

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

I think the underscore was to avoid considering this class as a Builtin to be shown in the documentation.

The entire module is marked no-doc. So this should no longer be a reason for having an underscore. In Python, an underscore typically indicates a private class, and this is not private. If it had an underscore previously, that was the wrong way to solve the problem. But at any rate this problem does not exist.

It should not be loaded as a Built-in symbol. Check what happens in the CLI when you entry

?BoxGraphicsElement

The underscore prevents that this class contribute with a definition. But this is a symptom that this module does not belongs to mathics.builtin.box module.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I get in this branch:

In[1]:= ? GraphicsElementBox
Out[1]= box representation for a graphics element
        
        Attributes[GraphicsElementBox] = {Protected, ReadProtected}

Copy link
Member Author

Choose a reason for hiding this comment

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

It should not be loaded as a Built-in symbol. Check what happens in the CLI when you entry

?BoxGraphicsElement

I think you mean ?GraphicsElementBox, right?

The underscore prevents that this class contribute with a definition. But this is a symptom that this module does not belongs here.

If it gets renamed with the word Box first, I don't think that will get added as a definition, right? I think that's how BoxExpression works, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should not be loaded as a Built-in symbol. Check what happens in the CLI when you entry
?BoxGraphicsElement

I think you mean ?GraphicsElementBox, right?

yep

The underscore prevents that this class contribute with a definition. But this is a symptom that this module does not belongs here.

If it gets renamed with the word Box first, I don't think that will get added as a definition, right? I think that's how BoxExpression works, right?

We was very clever if we did that...

Copy link
Contributor

Choose a reason for hiding this comment

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

As it is, it is loaded. But OK, it is not terrible. We can adjust it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just looked at the code. What is done to define list DOES_NOT_ADD_BUILTIN_DEFINITION = [...] in the module.

See https://github.com/Mathics3/mathics-core/blob/master/mathics/builtin/forms/base.py#L51

And that is far better than overloading the meaning of underscore at the beginning of a list. I'll add this in there now.

Initialize:
  * BoxExpression.boxes
  * GraphicsBox.boxwidth
  * GraphicsBox.boxheight
  * GrapnicsBox.boxes

Make sure to convert sympy.Float to Python float
@rocky
Copy link
Member Author

rocky commented Feb 3, 2026

I am seing now some type errors in master too, with the latest version of mypy (1.19.1). Some of the errors here should be fixed by adding an assertion / condition to narrow the type of the argument.

Ok.

But I guess that in all the render routines, the type of the argument box should be BoxElementMixin, and then narrow them inside each function.

I don't think "all", but just the top-level generic ones.

@rocky rocky force-pushed the add-box-type-annotations branch from 0fcab42 to 7325a25 Compare February 3, 2026 16:06
@mmatera
Copy link
Contributor

mmatera commented Feb 3, 2026

I don't think "all", but just the top-level generic ones.

I mean, all the functions which are registered.

@rocky
Copy link
Member Author

rocky commented Feb 3, 2026

I don't think "all", but just the top-level generic ones.

I mean, all the functions which are registered.

This is not the situation for RowBox, and several other boxes like that. def rowbox(box: RowBox..) should always get passed a RowBox.

@rocky rocky force-pushed the add-box-type-annotations branch from 7325a25 to 942752a Compare February 3, 2026 16:40
Copy link
Contributor

@mmatera mmatera left a comment

Choose a reason for hiding this comment

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

LGTM. Merge when you feel it is ready.

so ??GraphicsElementBox is not predefined inside a Mathics3 session.
@rocky rocky merged commit 3a11b75 into master Feb 3, 2026
21 checks passed
@rocky rocky deleted the add-box-type-annotations branch February 3, 2026 17:19
@rocky
Copy link
Member Author

rocky commented Feb 3, 2026

LGTM. Merge when you feel it is ready.

Great! Tomorrow, I'll look over the separation of box attributes, which belong to the box object, from boxing options, which are passed via the **options parameter.

After that, we can then start a discussion on the code that needs to compute box information and where and how that should work.

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