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

More operator tables #86

Merged
merged 6 commits into from
Nov 23, 2024
Merged

More operator tables #86

merged 6 commits into from
Nov 23, 2024

Conversation

rocky
Copy link
Member

@rocky rocky commented Nov 23, 2024

Most of the information we will need in mathics-core to revise mathics.core.parser.operators.

@rocky rocky requested a review from mmatera November 23, 2024 00:47
@rocky rocky force-pushed the more-operator-tables branch from ed2c240 to 7469472 Compare November 23, 2024 10:03
@@ -5284,7 +5311,7 @@ PlusMinus:

Postfix:
actual-precedence: 640
precedence: 640
precedence: 70 # Seems weird but this is what old Mathics code has
Copy link
Contributor

Choose a reason for hiding this comment

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

In WL it is 70, so I think it is correct

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 will remove the comment then. Thanks!

@@ -7839,7 +7866,7 @@ Xnor:

Xor:
actual-precedence: 280
precedence: 215
precedence: 220
Copy link
Contributor

Choose a reason for hiding this comment

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

In WL this is 215

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 will make note of this for the future in a comment FIXME.

The change to Mathics core which is coming up will be a bit large and probably painful for me. I will probably have to make several passes to get everything tidy.

But initially, I am going to be conservative and go with existing Mathics code. After things pass I can go back and fix up the list of discrepancies that were found.

meaningful: true
# comments:

UndirectedEdge:
actual-precedence: 370
precedence: 295
precedence: 120
Copy link
Contributor

Choose a reason for hiding this comment

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

In WL this is 295

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks again for checking. I will again make a TODO fixme for this.

Some will have to be FIXME's until we get everything merged in
and we can start a 2nd round of testing/adjusting.
@@ -168,15 +177,15 @@ AngleBracket:
Apply:
actual-precedence: 820
Precedence-Function: 620
precedence: 626
precedence: 620
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the criterium now is what Precedece[] says, isn't it?

Copy link
Member Author

@rocky rocky Nov 23, 2024

Choose a reason for hiding this comment

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

I am probably not using the terms in the way that Robert Jacobson had intended.

Right now, "precedence" is what mathics-core uses as precedence, whether it is Precendence[] or not.

Ideally, this would be the same as Precedence[] and to the extent we can make it that we should. Only in those cases where we find we can't, I envision the Precedence-Function to give the Precedence[] value. And where they are the same there is no need to include this field in the table duplicating the "precedence" value.

Down the line, the "actual-precedence" field can be removed.

However, right now I want to get something minimally working. Unfortunately, just this is already a big change. But a really good one! We will be much closer to providing everything the way it appears in WMA with reduced hand-coded values strewn about in Python code.

As I type this I have finally gotten Mathics-core to pass the regular tests using data from this YAML file. The manifest stuff will require more work.

Obviously this code has to get merged before mathics-core. So that will happen soon.

@mmatera
Copy link
Contributor

mmatera commented Nov 23, 2024

LGTM. @rocky, merge this when you feel it is ready.

This can now be used with upcoming code to Mathics-core which will
replace some of the operator-coded information in Python files.
However much more would be nice.
@rocky rocky merged commit a01834c into master Nov 23, 2024
12 checks passed
@rocky rocky deleted the more-operator-tables branch November 23, 2024 22:31
@rocky
Copy link
Member Author

rocky commented Nov 23, 2024

BTW - what remains in this repository is to add more of the operator symbols to the scanner in a way keyed from the JSON file. This will be coming up next.

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