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

Precedence for Directed and Undirected Edge #103

Merged
merged 9 commits into from
Dec 3, 2024

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 30, 2024

This PR sets the value of UndirectedEdge and DirectedEdge to 295. This value is the one reported in WMA by Precedence, and is compatible with the way in which expressions are parsed and formatted in WMA:

In[1]:=  Print["Precedences from Precedence[_]:"];
Print[ Table[op ->  Precedence[op], {op, {TildeTilde, UndirectedEdge, DirectedEdge,   SquareUnion}}]];
Print["Parsing:"];
(*Compare with the way in which expressions involving the operators are parsed:*)
Print["UndirectedEdge and DirectedEdge has higher precedence than TildeTilde:"];
Print[a \[TildeTilde] b \[UndirectedEdge] c, "  parsed as  ",   FullForm[a \[TildeTilde] b \[UndirectedEdge] c]];
Print[a \[UndirectedEdge] b \[TildeTilde] c, "  parsed as  ",   FullForm[a \[UndirectedEdge] b \[TildeTilde] c]];
Print[a \[TildeTilde] b \[DirectedEdge] c, "  parsed as  ",   FullForm[a \[TildeTilde] b \[DirectedEdge] c]];
Print[a \[DirectedEdge] b \[TildeTilde] c, "  parsed as  ",   FullForm[a \[DirectedEdge] b \[TildeTilde] c]];
Print["\nUndirectedEdge and DirectedEdge has the same precedence:"];
Print[a \[DirectedEdge] b \[UndirectedEdge] c, "  parsed as  ",   FullForm[a \[DirectedEdge] b \[UndirectedEdge] c]];
Print[a \[UndirectedEdge] b \[DirectedEdge] c, "  parsed as  ",   FullForm[a \[UndirectedEdge] b \[DirectedEdge] c]];
Print["\nUndirectedEdge and DirectedEdge has higher precedence than SquareUnion:"];
Print[a \[SquareUnion] b \[UndirectedEdge] c, "  parsed as  ",   FullForm[a \[SquareUnion] b \[UndirectedEdge] c]];
Print[a \[UndirectedEdge] b \[SquareUnion] c, "  parsed as  ",   FullForm[a \[UndirectedEdge] b \[SquareUnion] c]];
Print[a \[SquareUnion] b \[DirectedEdge] c, "  parsed as  ",   FullForm[a \[SquareUnion] b \[DirectedEdge] c]];
Print[a \[DirectedEdge] b \[SquareUnion] c, "  parsed as  ",   FullForm[a \[DirectedEdge] b \[SquareUnion] c]];
Print["Formatting"];
(*Compare with the way in which precedence is handled when formatting the expression with 
`Infix` in OutputForm:*)
Print[OutputForm[Infix[{a, b \[DirectedEdge] c}, "~", 294, None]]];
Print[OutputForm[Infix[{a, b \[DirectedEdge] c}, "~", 295, None]]];
Print[OutputForm[Infix[{a, b \[DirectedEdge] c}, "~", 296, None]]];
Print[OutputForm[Infix[{a, b \[UndirectedEdge] c}, "~", 294, None]]];
Print[OutputForm[Infix[{a, b \[UndirectedEdge] c}, "~", 295, None]]];
Print[OutputForm[Infix[{a, b \[UndirectedEdge] c}, "~", 296, None]]];


Precedences from Precedence[_]:
{TildeTilde -> 290., UndirectedEdge -> 295., DirectedEdge -> 295., SquareUnion -> 300.}

Parsing:

UndirectedEdge and DirectedEdge has higher precedence than TildeTilde:

a ≈ b  c parsed as TildeTilde[a, UndirectedEdge[b, c]]

a  b ≈ c parsed as TildeTilde[UndirectedEdge[a, b], c]

a ≈ b  c parsed as TildeTilde[a, DirectedEdge[b, c]]

a  b ≈ c parsed as TildeTilde[DirectedEdge[a, b], c]

UndirectedEdge and DirectedEdge has the same precedence:

a  b  c parsed as DirectedEdge[a, UndirectedEdge[b, c]]

a  b  c parsed as UndirectedEdge[a, DirectedEdge[b, c]]

UndirectedEdge and DirectedEdge has higher precedence than SquareUnion:

a ⊔ b  c parsed as UndirectedEdge[SquareUnion[a, b], c]

a  b ⊔ c parsed as UndirectedEdge[a, SquareUnion[b, c]]

a ⊔ b  c parsed as DirectedEdge[SquareUnion[a, b], c]

a  b ⊔ c parsed as DirectedEdge[a, SquareUnion[b, c]]

Formatting

a~b  c

a~b  c

a~(b  c)

a~b  c

a~b  c

a~(b  c)

@rocky
Copy link
Member

rocky commented Nov 30, 2024

We should be adding pytests for these. Thanks.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 30, 2024

Yep, I am thinking about how to implement it in general. I could add a test for this specific case, but what I would be testing is if mathics-core produces a consistent output for this particular case.

@rocky
Copy link
Member

rocky commented Nov 30, 2024

Yep, I am thinking about how to implement it in general.

Add be a section that picks infix operators at random (up to some number of times) in order to check associativity and combinability.

Likewise for prefix and postfix order.

This is one cool aspect that can be made use of now that operators are in a machine-readable table.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 30, 2024

Yep, I am thinking about how to implement it in general.

Add be a section that picks infix operators at random (up to some number of times) in order to check associativity and combinability.

Likewise for prefix and postfix order.

This is one cool aspect that can be made use of now that operators are in a machine-readable table.

OK, but we should check it against what? the Mathics output or the WMA output? What I am doing now is writing some WL code to do the consistency checks that I put in the description of this PR. Then, if we have doubts about a precedence value, we can check it in WMA and see if our guess is consistent with the WMA behavioir.

@rocky
Copy link
Member

rocky commented Nov 30, 2024

Yep, I am thinking about how to implement it in general.

Add be a section that picks infix operators at random (up to some number of times) in order to check associativity and combinability.
Likewise for prefix and postfix order.
This is one cool aspect that can be made use of now that operators are in a machine-readable table.

OK, but we should check it against what? the Mathics output or the WMA output? What I am doing now is writing some WL code to do the consistency checks that I put in the description of this PR. Then, if we have doubts about a precedence value, we can check it in WMA and see if our guess is consistent with the WMA behavioir.

Check against Mathics with an option to save locally the test cases generated. Periodically, run the unique collected test cases against WMA. (At that time you can also check for how random the tests are by how unique the collected tests are.)

@mmatera
Copy link
Contributor Author

mmatera commented Nov 30, 2024

Yep, I am thinking about how to implement it in general.

Add be a section that picks infix operators at random (up to some number of times) in order to check associativity and combinability.
Likewise for prefix and postfix order.
This is one cool aspect that can be made use of now that operators are in a machine-readable table.

OK, but we should check it against what? the Mathics output or the WMA output? What I am doing now is writing some WL code to do the consistency checks that I put in the description of this PR. Then, if we have doubts about a precedence value, we can check it in WMA and see if our guess is consistent with the WMA behavioir.

Check against Mathics with an option to save locally the test cases generated. Periodically, run the unique collected test cases against WMA. (At that time you can also check for how random the tests are by how unique the collected tests are.)

OK, but the tests against Mathics are tests over the consistency of our Parser and our Formatter, which are in mathics-core.

@rocky
Copy link
Member

rocky commented Nov 30, 2024

OK, but the tests against Mathics are tests over the consistency of our Parser and our Formatter, which are in mathics-core.

I don't understand what this means.

Parsing other than of operator precedence should be done in mathics-core. Here, we are just focusing on operator precedence.

Having CI compare the behavior by running WMA requires CI to have a WMA license, which I don't think is easily possible right now. If you want to add a way to compare against WMA, okay, but it would probably need to run outside of CI.

Personally, I think that right now this is a lot of complexity and effort for very little gain. Should someone encounter a real problem, there is the issue tracker. Worrying about how RoundedImplies works before encountering a package that uses this as a binary infix operator next to another binary infix operator is not a great use of time.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 30, 2024

OK, but the tests against Mathics are tests over the consistency of our Parser and our Formatter, which are in mathics-core.

I don't understand what this means.

Parsing other than of operator precedence should be done in mathics-core. Here, we are just focusing on operator precedence.

Having CI compare the behavior by running WMA requires CI to have a WMA license, which I don't think is easily possible right now. If you want to add a way to compare against WMA, okay, but it would probably need to run outside of CI.

Right, this is why I collected the information using the licence I have for Wolframscript, and just put it inside a test.

Personally, I think that right now this is a lot of complexity and effort for very little gain. Should someone encounter a real problem, there is the issue tracker. Worrying about how RoundedImplies works before encountering a package that uses this as a binary infix operator next to another binary infix operator is not a great use of time.

I do not want to go much beyond this, and #104. The idea was just to have some tools to detect issues when a package like Ruby fails to load or shows a weird behavior, related to these parsing/formatting parameters.

@rocky
Copy link
Member

rocky commented Dec 2, 2024

@mmatera if we add some tests, this could get merged.

# In Mathics, the precedence of these operators is quite low.
# "DirectedEdge", # Mathics 128 , WMA 295
# "UndirectedEdge", # Mathics 120, WMA 295
"DirectedEdge",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The precedence order of these symbols is tested here. Of course, other tests can be added.

Copy link
Member

Choose a reason for hiding this comment

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

This test is extremely opaque. (BTW the one for TestConstrained is also that way.)

I was hoping for something operational or something a user might experience, rather than checking that the value in this table is less than another value.

Setting operator precedence determines whether we have parenthesis or not. So in a chain of direct/directed operators, we should see in full form the grouping change, and when there are other operators around them, then either parenthesis or the way calls are nested change.

I seem to recall that there were tests around that worked that way. This is testing the effect of the precedence values on the way parsing happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is extremely opaque. (BTW the one for TestConstrained is also that way.)

OK, but this is what we can check without bitting our tails: we cannot deduce what the precedence order is for a given pair of operators: we need to look at a reference. The parser or the formatter of Mathics-core could show the right behavior without looking at the tables. And even if they do, checking that their behavior is consistent with the information in the table is not a warranty that this information is right, because it was produced from it: What we are checking is the parser and the formatter, not the accuracy of the information.

What I think it makes more sense is to check if the precedence order in WMA is consistent with its own behavior, and handle the inconsistencies by slightly adjust the values that we store. This is why I wrote this kind of operational test in a WL module. Then, if someone has doubts about one precedence value in the table, can go to wolframscript and check it out. Of course, we can also use it over mathics-core.

Maybe it worth to put this in a comment on the pytest module, but it would go in another PR.

Copy link
Member

@rocky rocky Dec 3, 2024

Choose a reason for hiding this comment

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

The initial commentary for this PR shows WMA code that was run demonstrating why the precedence needs to be changed.

I want the tests to reflect that by doing the same thing. In general, if one observes that something is wrong because when I run something I get this, then fixing that should run the demonstration to show this is now addressed.

In the future, developers are more likely to encounter the pytest code rather than some PR commentary. The PR commentary is not checked in a simple-minded and easy-to-understand way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but the comments have the only purpose to converge to some criteria about what value must be assigned. Once these criteria can be clearly written, the comments can go out. I started to do that in #105, over this.

Copy link
Member

@rocky rocky Dec 3, 2024

Choose a reason for hiding this comment

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

#105 is fine (with one small removal of slight vagueness).

This PR changes the operator precedence numerical value of DirectedEdge and UndirectedEdge. I am asking that a test be written to demonstrate why the operator precedence change in value is needed and to check that the operational effect we want to achieve persists in the future.

* removing the numerical comparison test. Adding an explanation at the beginning of the file

* fixing the name of the constant. Improving docstrings

* black

* codespell
@mmatera
Copy link
Contributor Author

mmatera commented Dec 3, 2024

Now I have added some "operational" tests. These tests reveal more about the differences between how mathics-core and WMA parse and format expressions than whether the precedence valures are correct.

@rocky
Copy link
Member

rocky commented Dec 3, 2024

Now I have added some "operational" tests.

Thank you very much! I appreciate your patience, understanding, and tolerance.

These tests reveal more about the differences between how mathics-core and WMA parse and format expressions than whether the precedence values are correct.

Yes, that may be the case, but the problem is seen here in adjusting precedence. I would be grateful if you would open some bug tickets in mathics-core to address these.

As the issues are fixed in mathics-core, we can remove the tests here and add them to mathics-core.

@rocky rocky merged commit 1d8a90c into master Dec 3, 2024
12 checks passed
@rocky rocky deleted the precedence_Undirected_and_Directed branch December 3, 2024 15:05
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