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

test precedences against WMA #1192

Closed
wants to merge 8 commits into from
Closed

test precedences against WMA #1192

wants to merge 8 commits into from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 27, 2024

This PR adds a Pytest module to compare the precedences with the ones obtained in WMA.

@rocky
Copy link
Member

rocky commented Nov 27, 2024

While it is a good idea to check operator precedence against WMA, I am trying to understand why this is a pytest and what we might use it for.

If the goal is to get Mathics3's operator precedence closer to WMA's that's fine, but this is more like a one-time and very infrequent kind of thing to do.

I believe right now we could set the precedence for "Definiton" and "Information" to almost what WMA has. But we might not be able to match both exactly, and that is why we have another entry to mark the discrepancy in the operators.yml file.

If I have it right, WMA has these two as the same value. But for us, it may be that we can't have these be same value or else the scanner/parser combo will treat "??" as "? ?". That said, probably one of those values can probably be what WMA has. I suspect that the high value that was set in Mathics3 was done more out of "quick fix" desperation, than careful cool consideration.

And also, when we find a need for the difference, it would be nice to have a unit test that pinpoints exactly these kinds of situations. I started to add comments in the YAML file, but a working test is, of course, better.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 28, 2024

@rocky, indeed, this is just a way to test the compatibility between the mathics-scanner tables and what WMA says about the precedence of these operators. I put this here not with the aim of merging it, but just to check what the differences are, as we make progress in using the information Mathics-scanner tables in Mathics core.

And I agree: checking the coincidence in numbers is probably not the best way to check it. A better idea would be to check the format that results of combining pairs of these operators. For example, in many cases the relation
Precedence[op1]<=Precedence[op2]
is equivalent to check that
ToString[op1[op2[a,b],c], InputForm]
produce a string without parenthesis. This kind of test would be more robust than the one in this PR, but requires to handle details like if the operator is represented as an Infix form, or as a kind of bracket, or if the operator is binary or unary.
When I finish with it, I could compare the induced order relation against the same order relation in Mathics.

Regarding whether this is a Pytest or another kind of test, is a different question, to face once the tests check what we want to check.

@rocky
Copy link
Member

rocky commented Nov 28, 2024

@rocky, indeed, this is just a way to test the compatibility between the mathics-scanner tables and what WMA says about the precedence of these operators. I put this here not with the aim of merging it, but just to check what the differences are, as we make progress in using the information Mathics-scanner tables in Mathics core.

And I agree: checking the coincidence in numbers is probably not the best way to check it. A better idea would be to check the format that results of combining pairs of these operators. For example, in many cases the relation Precedence[op1]<=Precedence[op2] is equivalent to check that ToString[op1[op2[a,b],c], InputForm] produce a string without parenthesis. This kind of test would be more robust than the one in this PR, but requires to handle details like if the operator is represented as an Infix form, or as a kind of bracket, or if the operator is binary or unary. When I finish with it, I could compare the induced order relation against the same order relation in Mathics.

Regarding whether this is a Pytest or another kind of test, is a different question, to face once the tests check what we want to check.

Ok. Thanks for the information. This helps me understand what your intent is and what you are concerned about.

This activity surprised me because it felt done out of order: checking in advance of attempting to reduce known gratuitous differences. However, this was my bad in that I did not make it clearer that I was aware of what I think are unnecessary differences.

If you would like to do that though, by all means, go ahead! I added a couple of comments in the YAML, where I thought something was weird, but I can only recall the Definition and Information values. I think there are other cases as well though.

So then that bring us about what how we should handle Mathics3 Precedence[]. Should it be ours, or be compatible with WMA? I am guessing what we have (which is why would be nice to reduce and minimize unnecessary differences).

There could be an option of Precedence[] to show the other precedence value whichever way we go.

@rocky
Copy link
Member

rocky commented Nov 29, 2024

The observations and conclusions should eventually go as comments in the operator YAML file in mathics scanner.

In fact, all of this might be done in the mathics scanner scanner repo. And we could keep code in the generate directory as well.

Finally, since that repository changes less, there would be fewer rebases that might go on.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 29, 2024

The observations and conclusions should eventually go as comments in the operator YAML file in mathics scanner.

In fact, all of this might be done in the mathics scanner scanner repo. And we could keep code in the generate directory as well.

Finally, since that repository changes less, there would be fewer rebases that might go on.

I Agree. I will try to move this to Mathics-scanner when I finish it.

@@ -136,13 +136,13 @@ def test_private_doctests_datetime(str_expr, msgs, str_expected, fail_msg):
[
##
(
"TimeConstrained[Integrate[Sin[x]^100,x],.001]",
"TimeConstrained[Integrate[Sin[x]^1000,x],.001]",
Copy link
Member

Choose a reason for hiding this comment

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

This should be in its own PR. Also, the fact that we have to constantly adjust this suggests that we are doing this the wrong way.

There is probably a way using Python's mock library that would accomplish what we want to do more reliably and faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I know, but since this is a draft branch, I take the chance to make experiments...

Copy link
Member

@rocky rocky Nov 29, 2024

Choose a reason for hiding this comment

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

Can't you set up a private fork of the repository and set up github workflows on that and do experiments this way?

You tend to work in a very noisy way - use the most watched repo of the organization, make lots of little commits that don't have much public value nor have meaningful commit messages.

I confess I will tend to do this myself on smaller lesser followed libraries, so yes, it is expedient. But noisy.

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, right, I'll do this in my github fork.

@mmatera
Copy link
Contributor Author

mmatera commented Nov 29, 2024

Now the tests were moved into Mathics3/mathics-scanner#99

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