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 tests #99

Closed
wants to merge 7 commits into from
Closed

Precedence tests #99

wants to merge 7 commits into from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 29, 2024

This includes the tests proposed in Mathics3/mathics-core#1192 where it should belong.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 29, 2024

Whether the tests could be implemented at a lower level, just looking into the tables, I want to include later some tests checking how certain expressions are parsed and formatted when the precedence values are taken into account, so I need Mathics-core for that.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 29, 2024

For some reason, from this side the precedence values for Nand and Nor are not readed.

@rocky
Copy link
Member

rocky commented Nov 29, 2024

For some reason, from this side the precedence values for Nand and Nor are not readed.

Are you working from a cloned Github repository instead of the last released code?

This is probably something that can be easily debugged.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 29, 2024

No, the problem was that the github action workflow uses the operator-info-from-JSON branch of Mathics-core. Once I updated the branch, the test passed.

"LeftTee",
"Perpendicular", # 190
"RightTee", # 190
# In WMA, `RoundImplies` has a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably need to adjust the precedence values in the tables for UpTee and RoundImplies

Copy link
Member

Choose a reason for hiding this comment

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

"TildeEqual",
"TildeFullEqual",
"TildeTilde",
# In Mathics, the precedence of these operators is quite low.
Copy link
Contributor Author

@mmatera mmatera Nov 29, 2024

Choose a reason for hiding this comment

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

Is there a reason to choose these lower values for the precedence of these operators?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to choose these lower values for the precedence of these operators?

See #99 (comment)

"CircleDot",
"SmallCircle",
"Square", # 540
"Del", # In WMA, has the same precedence as DifferentialD and CapitalDifferentialD
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see the need of using a different value of precedence for these operators.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Put in a isolated PR for this change along with changing the test to work from master. If the tests succeed, we can merge that in and delete operators_from_JSON branch in mathics-core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is just about the tests. Here I just wanted to leave the comments.

"PlusMinus",
"Plus",
"Subtract", # 310
# "Integrete", # In Mathics, this has the default precedence. In WMA, 325
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point, we need to look over again the way how the information for Integral and Integrate operators are stored in the tables..

@rocky
Copy link
Member

rocky commented Nov 29, 2024

No, the problem was that the github action workflow uses the operator-info-from-JSON branch of Mathics-core. Once I updated the branch, the test passed.

Ah, that was a step I forgot to do after the branch got merged.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 30, 2024

The tests are already merged

@mmatera mmatera closed this Nov 30, 2024
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