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

An example of defining MakeBoxes in Mathics code #590

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rocky
Copy link
Member

@rocky rocky commented Nov 2, 2022

We hack assigment for MakeBoxes, but ultimately this is wrong.

There seems to be confusion or vagueness in MakeBoxes internals with respect to Rules and delayed assignment.

@rocky rocky requested a review from mmatera November 2, 2022 10:20
@rocky rocky marked this pull request as draft November 2, 2022 10:21
@rocky
Copy link
Member Author

rocky commented Nov 2, 2022

This code has problems, but I hope it conveys an idea for pulling out Boxing rules organized by Form and reading this from a Mathics file.

Reading from a file in Mathics right now is slow. But should and will eventually address that.

Please just commit over this to fill it out.

# MakeBoxes[CubeRoot, StandardForm] := RadicalBox[3, StandardForm]
# rather than:
# MakeBoxes[CubeRoot, StandardForm] -> RadicalBox[3, StandardForm]
elif lhs.get_head_name() == "System`MakeBoxes":
Copy link
Contributor

Choose a reason for hiding this comment

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

If you look above, you can see that some time ago we had defined a dictionary where special kinds of LHS are associated yo different ways to perform the assignment. Maybe it would be wise to use it.

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 don't see exactly what you are talking about and this code is all temporary until the set functions are gone over. And there may be something even larger wrong in the way contribute() works if it works using the rules class variable.

That said, if you feel you have a better and cleaner way to do this, by all means go head and commit on top.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is just a matter of organization. And probably, you would like to reformulate it again. However, I think that the idea of using a dictionary to store the connection between different kinds of expressions and implementation was an improvement over a large spaghetti code.

if not isinstance(pattern, BaseElement):
assert pattern
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it happen? It shouldn't, because the pattern comes from a rule object.

Copy link
Member Author

@rocky rocky Nov 2, 2022

Choose a reason for hiding this comment

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

I had intended to write assert isinstance(pattern, str). And that would have been added just for my own understanding. I have removed it in 857c7e1.

As mentioned in the comment above this section of the code, I find weird that sometimes variable "pattern" is a string and sometimes it is a Rule. And then we have the type-shifting behavior of the variable getting converted from a string to a Rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this code again, in line 286 pattern is always a string, since is the key of the Builtin.rules dict. So the if condition is always trivially true. So, I would change the for block for this another code:

        for pattern_str, replace in self.rules.items():
            pattern_str = pattern_str % {"name": name}
            pattern = parse_builtin_rule(pattern_str, definition_class)
            replace = replace % {"name": name}
            rules.append(Rule(pattern, parse_builtin_rule(replace), system=not is_pymodule))

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok - thanks for investigating and suggesting a fix! Would you just carry on and add this? Tests fail and I don't understand why.

I think it would be cool to have a more complete set of rules for the different forms, StandardForm, OutputForm, and so on accessible in Mathics.

As mentioned before if file loading is too slow, we might investigate why and address that as a separate problem which probably needs to be fixed anyway since we want users to be able to load files quickly too.

Copy link
Contributor

Choose a reason for hiding this comment

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

The tests thar are failing seems to be related to definitions that are not removed after previous tests. In particular, the tests that are failing are:

  • Formatting Output in Manual / Language Tutorial
  • Names in Reference of Built-in Symbols / Atomic Elements of Expressions
  • TeXForm in Reference of Built-in Symbols / Forms of Input and Output
  • Integrate in Reference of Built-in Symbols / Integer and Number-Theoretical Functions
  • Association in Reference of Built-in Symbols / List Functions
  • ToBoxes in Reference of Built-in Symbols / Low level Format definitions
  • Subsuperscript in Reference of Built-in Symbols / This module contains symbols used to define the high level layout for

However, except for the first one, the other tests pass if I run them separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok - if you know how to fix please do.

The questions is why there is that special case table. Is there some broad pattern to it? If so, what?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, at some point we had identified a pattern that applies for these cases:

    special_cases = {
        "System`$Context": process_assign_context,
        "System`$ContextPath": process_assign_context_path,
        "System`$RandomState": process_assign_random_state,
        "System`Attributes": process_assign_attributes,
        "System`Default": process_assign_default,
        "System`DefaultValues": process_assign_definition_values,
        "System`DownValues": process_assign_definition_values,
        "System`Format": process_assign_format,
        "System`MakeBoxes": process_assign_makeboxes,
        "System`MessageName": process_assign_messagename,
        "System`Messages": process_assign_definition_values,
        "System`N": process_assign_n,
        "System`NValues": process_assign_definition_values,
        "System`NumericQ": process_assign_numericq,
        "System`Options": process_assign_options,
        "System`OwnValues": process_assign_definition_values,
        "System`SubValues": process_assign_definition_values,
        "System`UpValues": process_assign_definition_values,
    }

In these special cases, we are not looking for a rule in a definition in the normal way. For example, N[F[x_]]:=x^2 do not stores the rules in the downvalues of N, but in the nvalues of F. System$Context=...does not just assigns anownvalueto the System$Context symbol, but also changes an internal variable in the Definitions object,
and setting OwnValues[a]:=x does not store a downvalue of OwnValues, but sets an ownvalue on a.

Copy link
Member Author

Choose a reason for hiding this comment

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

is it fair to say that the kind of assignment done is based on properties of Head?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the current state, yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then perhaps we should be testing on properties head rather branching based on specific Symbol names.

rocky and others added 5 commits November 6, 2022 14:51
We hack assigment for MakeBoxes, but ultimately this is wrong.

There seems to be confusion in MakeBoxes internals with respect
to Rules and delayed assignment.
My WL skills are not that good.
* add `Remove`. Fix load definitions. Normalize assignment for MakeBoxes

* support strings
also, merge accidently reverted a "ssym" removal
@mmatera
Copy link
Contributor

mmatera commented Nov 7, 2022

I think this is ready to merge now. Also, I still can split the remaining parts with the fixes that makes that the central part of this works.

@rocky
Copy link
Member Author

rocky commented Nov 7, 2022

I think this is ready to merge now.

Have you tried this changing RadBox to RadicalBox and see that the result is formatted properly?

A while ago I tried, but I am not sure I found the specific example that convinced me that everything worked okay.

Also, I still can split the remaining parts with the fixes that makes that the central part of this works.

That would be awesome! As things grow I think we want to separate into a file the common parts. Here, CubeRootRadixBox is the routine that is common among several Forms, of which we only have one using it. I am not that proficient to know how to have that common file included only once

After this is done, then yes, I think it is ready for merge, and I look forward to seeing what the complete Forms look like in Mathics code.

I'll also say, that I suspect there will be more work how MakeBoxes is implemented. But that can come later and is independent.

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.

2 participants