Skip to content

More suggestions and fixes #595

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 6, 2022
Merged

More suggestions and fixes #595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df45e8c
my suggestions
mmatera Nov 4, 2022
01b51b1
add `Remove`. Fix load definitions. Normalize assignment for MakeBoxes
mmatera Nov 6, 2022
eedac49
merge
mmatera Nov 6, 2022
f46d0bc
support strings
mmatera Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ New Builtins
#. ``Curl`` (2-D and 3-D vector forms only)
#. ``Kurtosis``
#. ``PauliMatrix``
#. ``Remove``
#. ``SixJSymbol``
#. ``Skewness``
#. ``ThreeJSymbol``
Expand Down
1 change: 1 addition & 0 deletions SYMBOLS_MANIFEST.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,7 @@ System`RegularExpression
System`RegularPolygon
System`RegularPolygonBox
System`ReleaseHold
System`Remove
System`RemoveDiacritics
System`RenameDirectory
System`RenameFile
Expand Down
2 changes: 1 addition & 1 deletion mathics/autoload/forms/StandardForm.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
code. *)
Attributes[CommonRadicalBox] = HoldAll;
Attributes[RadBox] = HoldAll;
CommonRadicalBox[expr_, form_] = RadBox[MakeBoxes[expr, form], 3];
CommonRadicalBox[expr_, form_]:= RadBox[MakeBoxes[expr, form], 3];
Copy link
Member

@rocky rocky Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I know this may be wordy, but how about RadicalCubeRootBox?

I made a mistake here in using the generic term "Radical" when what we are referring to is is a cube root radical.

"radical" box" by itself is too general because this is not a rule for a square root, and "cube box" is too general too because exponent 1/3 is a cube root as well, so what we want to convey is the radical form of a cube root.

Note that in the names here, we are referring somewhat to the visual form, e.g. "radical" boxing as opposed to "power" boxing even though many times there is a box that surrounds the argument.

The name though will influence formatting later on.

Copy link
Contributor Author

@mmatera mmatera Nov 6, 2022

Choose a reason for hiding this comment

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

At this point, wordy is OK.


(******************************************************************************************)
(* StandardForm Boxing Rules *)
Expand Down
30 changes: 30 additions & 0 deletions mathics/builtin/assignments/clear.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,36 @@ def do_clear(self, definition):
definition.defaultvalues = []


class Remove(Builtin):
"""
<dl>
<dt>'Remove[$x$]'
<dd>removes the definition associated to $x$.
</dl>
>> a := 2
>> Names["Global`a"]
= {a}
>> Remove[a]
>> Names["Global`a"]
= {}
"""

attributes = A_HOLD_ALL | A_LOCKED | A_PROTECTED
messages = {"ssym": "`1` is not a symbol."}
Copy link
Member

@rocky rocky Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I see "ssym" defined a number of places:

git grep -n '"ssym:"'
mathics/builtin/assignments/clear.py:80:        "ssym": "`1` is not a symbol or a string.",
mathics/builtin/attributes.py:209:        "ssym": "`1` is not a symbol or a string.",
mathics/builtin/attributes.py:265:        "ssym": "`1` is not a symbol or a string.",

I think if we just add it to mathics-core/mathics/builtin/messages.py we can DRY the code:

Copy link
Contributor Author

@mmatera mmatera Nov 6, 2022

Choose a reason for hiding this comment

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

Yep.

Copy link
Contributor Author

@mmatera mmatera Nov 6, 2022

Choose a reason for hiding this comment

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

Let's do it in the next round.

precedence = 670
summary_text = "remove the definition of a symbol"

def eval(self, symb, evaluation):
"""Remove[symb_]"""
if isinstance(symb, Symbol):
evaluation.definitions.reset_user_definition(symb.name)
elif isinstance(symb, String):
evaluation.definitions.reset_user_definition(symb.value)
else:
evaluation.message(self.get_name(), "ssym", symb)
return SymbolNull


class Unset(PostfixOperator):
"""
<dl>
Expand Down
9 changes: 6 additions & 3 deletions mathics/builtin/assignments/internals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,9 +567,12 @@ def process_assign_makeboxes(self, lhs, rhs, evaluation, tags, upset):
# MakeBoxes[CubeRoot, StandardForm] := RadicalBox[3, StandardForm]
# rather than:
# MakeBoxes[CubeRoot, StandardForm] -> RadicalBox[3, StandardForm]
makeboxes_rule = Rule(lhs, rhs, system=True)
makeboxes_defs = evaluation.definitions.builtin["System`MakeBoxes"]
makeboxes_defs.add_rule(makeboxes_rule)

makeboxes_rule = Rule(lhs, rhs, system=False)
definitions = evaluation.definitions
definitions.add_rule("System`MakeBoxes", makeboxes_rule, "down")
Copy link
Member

@rocky rocky Nov 6, 2022

Choose a reason for hiding this comment

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

Is the reason, "add_rule" takes a string here a historical artifact, or is there a reason why a string is better than a Symbol? Semantically, Symbol is the more precise entity, right?

Copy link
Contributor Author

@mmatera mmatera Nov 6, 2022

Choose a reason for hiding this comment

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

Symbol would be more precise, but now, the key in Definitions is a Python str. To change it by a Symbol is not trivial (I tried once). So, the answer is both: historical and efficiency reasons.

# makeboxes_defs = evaluation.definitions.builtin["System`MakeBoxes"]
Copy link
Contributor Author

@mmatera mmatera Nov 6, 2022

Choose a reason for hiding this comment

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

During a session, we should not touch the builtin definition of a symbol, but ask to the Definitions object to add the new rule in the proper place. In general, the proper place is in an user definition, which is then merged with the builtin definition when it is needed.

Copy link
Member

@rocky rocky Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. Thanks for the explanation and fix of what is wrong.

Why is is there a definition inside the symbol at all? My gut feeling is that if this is removed, then Symbol is more like a Lisp Symbol (and we start to see more speed improvement ;-). But to start out with, let me narrow this question for just SymbolMakeBoxes symbols.

The definitions we are adding are (or will be) the first ones added, so what is the value of having any rule associated with SymbolMakeBoxes?

Copy link
Contributor Author

@mmatera mmatera Nov 6, 2022

Choose a reason for hiding this comment

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

I am not sure to understand your question. A Symbol in WL is a literal Atom, just like a String or an Integer. With all of them, you can build Expression s, which are just a list of Atoms.

Then we have Definitions, that we can associate to Symbols. We could have more than one Definition for the same Symbol. A Definition stores the attributes of a Symbol, together with a set (or several sets) of replacement rules.

Then, we have certain actions we can perform over these elements (Atoms and Expressions). Replacements are one of these the possible actions. In a replacement, we take an element, and we produce a new element following certain rules, and eventually, producing certain side actions on the system (like write a file, run a system command).

Another kind of actions that we can perform over an Expression is to "evaluate" it. For doing it,

Ok. Thanks for the explanation and fix of what is wrong.

Why is is there a definition inside the symbol at all? My gut feeling is that if this is removed, then Symbol is more like a Lisp Symbol (and we start to see more speed improvement ;-). But to start out with, let me narrow this question for just SymbolMakeBoxes symbols.

OK. I am not sure to understand the question. I will try again to provide a description of the elements, and probably you can reformulate them in more standard/generic/LISP-compliant terms.

Symbols do not have a Definition "inside". A Symbol is one kind of the Atom (ic) Elements over which Expressions are made. Elements and Expressions are the basic data type of the WL.
Elements have several methods that requires a set of definitions as arguments, but if we strip them, I think we could have a minimal "lexical" module which do not need to make any references to Definitions or Evaluation classes, where the state of a (Mathics) Session is stored.

Then, we can associate a Definition with a given Symbol. A Definition stores certain attributes and replacement rules associated to a Symbol and to the expression built from these symbols. Then, an Evaluation (evaluation context? / session context / command line session) is defined by a set of Definitions (a Definitions object) and other internal registers (timeouts, semaphores, expected formats, character encoding, etc, which for some reason we do not want to store in the form of Definitions).

Over an Expression, we can perform different actions that produce a new Expression. One possible action could be to sort/ rearrange the elements of an expression, or to replace a Symbol with another Element.
Another example would be to apply a Rule. In this kind of action, a part of an Expression that matches a certain pattern is replaced by another expression that can depend on the matching expression. If the rule is successfully applied, then we get a new Expression. In the process of building the new expression, the old expression could have side effects, like running a system command, writting a file, or changing the state of the session.

To evaluate an Expression consists then in take it, and applying interactively and recursively the replacement rules associated to the Symbols appearing in the Expression in a certain order, to produce a new Expression. To help picking the right/pertinent rules during the evaluation, each Definition has different sets of rules (Ownvalues, Downvalues, Upvalues, NValues, Format, Default, etc.).

If we can think about a unique evaluation loop, certain symbols lead to specific evaluation patterns, involving following a different order in the application of the rules. One example are the expressions of the form N[...]. These expressions look at a different set of rules (NValues) instead of the one used in a regular evaluation (Ownvalues, Downvalues, Upvalues, etc). Format and MakeBoxes does something similar.

The definitions we are adding are (or will be) the first ones added, so what is the value of having any rule associated with SymbolMakeBoxes?

In the current implementation, the evaluation of MakeBoxes[...] follows the same mechanism that the used for any other expression. And these kind of expressions are evaluated after the evaluation of any other Expression, even if it returns a Symbol or a number. The result of evaluating a MakeBoxes[] expression should be a BoxElementMixin object, which then can be converted into a (Python's) str which can be shown in the screen.

In Master, all the MakeBoxes rules are stored as builtin rules. In Master, if we set a MakeBoxes rule in an autoload file, these other definitions are erased, and the evaluation gets broken.

Copy link
Member

@rocky rocky Nov 6, 2022

Choose a reason for hiding this comment

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

I am not sure to understand your question. A Symbol in WL is a literal Atom, just like a String or an Integer. With all of them, you can build Expression s, which are just a list of Atoms.

Let me review how the code works now, used to work, look at WMA docs and so on. It may take a little time though.

# makeboxes_defs.add_rule(makeboxes_rule)
return True


Expand Down
9 changes: 8 additions & 1 deletion mathics/core/definitions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,14 @@ def __init__(
if name.startswith("Global`"):
raise ValueError("autoload defined %s." % name)

self.builtin.update(self.user)
# The idea here is that all the symbols loaded in
# autoload become converted in builtin.
# For some reason, if we do not do this here,
# `Export` and `Import` fails.
# TODO: investigate why.
Copy link
Member

@rocky rocky Nov 6, 2022

Choose a reason for hiding this comment

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

Is it a fair hypothesis that if Export and Import fail, it may be because the are looking inside the builtin rather than the user definitions?

Copy link
Contributor Author

@mmatera mmatera Nov 6, 2022

Choose a reason for hiding this comment

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

It is...

for name in self.user:
self.builtin[name] = self.get_definition(name)

self.user = {}
self.clear_cache()

Expand Down