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.

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

1 / Fix issues with pattern matching for Rubi #1176

Merged
merged 10 commits into from
Nov 23, 2024
Merged

1 / Fix issues with pattern matching for Rubi #1176

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
85ad0f5
Fix pattern matching: pop optionals.
aravindh-krishnamoorthy Nov 19, 2024
c1999ce
Fix optional patterns without default values for the given positions.
aravindh-krishnamoorthy Nov 19, 2024
e1dee54
Fix optional patterns without default values: Newpattern only if not …
aravindh-krishnamoorthy Nov 19, 2024
0a89945
Do not allow optional patterns for more than one argument.
aravindh-krishnamoorthy Nov 19, 2024
eff7ebc
Add tests for the fixes.
aravindh-krishnamoorthy Nov 22, 2024
bac8e03
Run isort and black.
aravindh-krishnamoorthy Nov 22, 2024
e1d342f
Merge branch 'master' into pattern-fix-rubi
aravindh-krishnamoorthy Nov 22, 2024
0f9786a
Add extended tests for function patterns.
aravindh-krishnamoorthy Nov 22, 2024
ae7ea50
Minor housekeeping.
aravindh-krishnamoorthy Nov 22, 2024
8a7a564
Move Mathics tests to toplevel; rewrite mathics.eval.patterns tests i…
aravindh-krishnamoorthy Nov 23, 2024
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
5 changes: 2 additions & 3 deletions test/core/test_patterns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# -*- coding: utf-8 -*-
"""
Unit tests for mathics pattern matching
Unit tests for mathics.core.pattern
Copy link
Member

@rocky rocky Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here is the part that feels a little weird to me. We are testing a lower-level function using higher-level tests. I am a little bit sensitive to this kind of thing, because this less-good practice is pervasive in this code.

At one time, we didn't have pytests. There were doctests that took the place of pytests. It took us a while to get unburied from that practice.

The MatchQ tests to me should be put in test.buitin.testing_expressions.expression_tests and the rule tests put wherever rules are located.

Tests here could (and should be) to build a ExpressionPattern and run match on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. Indeed, I can do that if you and mmatera prefer that... But that would be more of a Python test than Wolfram syntax, unlike the current tests...

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the part that feels a little weird to me. We are testing a lower-level function using higher-level tests. I am a little bit sensitive to this kind of thing, because this less-good practice is pervasive in this code.

At one time, we didn't have pytests. There were doctests that took the place of pytests. It took us a while to get unburied from that practice.

The MatchQ tests to me should be put in test.buitin.testing_expressions.expression_tests and the rule tests put wherever rules are located.

Tests here could (and should be) to build a ExpressionPattern and run match on it.

Yes, but in any case, at some point we can rewrite these tests as lower-level tests. I do not think that these tests actually test the behavior of MatchQ or ReplaceAll, but the low-level code. I can do the translation later, when I find some time for it.

Copy link
Member

@rocky rocky Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tests in test.builtin are testing Mathics3 Builtin functions. Of course, any time one tests a higher-level function one is necessarily testing all sorts of lower-level functions. So all of these tests are valid, it is just organized, in my opinion, in the wrong place.

Tests of the ExpressionPattern match function are good because they isolate testing that from all of the other kinds of stuff, and we pinpoint the problems better. when there is a problem. A MatchQ test could fail because of something totally unrelated to the ExpressionPattern match function.

Another thing that these smaller unit tests do (which shouldn't be needed but in fact is used way too much) is to show developers how one can work with, test, and debug the smaller units. Ideally, if our APIs and modularity are done well, the code needed to do this is not that much. Personally, I have found ways to improve modularity when I have been forced to these smaller parts in isolation.

Copy link
Member

@rocky rocky Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but in any case, at some point we can rewrite these tests as lower-level tests. I do not think that these tests actually test the behavior of MatchQ or ReplaceAll, but the low-level code. I can do the translation later, when I find some time for it.

My experience is that the rewrite tends to happen at a smaller rate than creating more code that is somewhat ill-conceived. In fact, I am in the middle of a larger rewrite, for operators right now. It is not fun. In fact, this aspect is what on the makes me the least happy about working on this project as compared to others.

If you want to do the minimal thing, then move the MatchQ code to where MatchQ should be tested and the ReplaceAll to where ReplaceAll tests are tested. At least, this honestly reflects what the test is doing. If later you want to rewrite, then at that time you can.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we test the evaluation of an expression like MatchQ[1+2 a, x_.+y_.*z_Symbol], many processes are involved: the main evaluation loop, the definitions of Plus and Times and their internal implementation, the pattern matching done in the evaluation loop, and eventually, the implementation of matchQ and the pattern matching mechanisms.
If we want to know if the pattern matching works as it should, it would be better to use mathics.eval.patterns.match, which does not try to evaluate its arguments, but just implements the pattern matching.
I think that this is the right level of testing for these tests.

Lower levels involve testing whether the functions and methods used to evaluate matching do what they are supposed to. But at this point, there are functions for which I am still unable to say exactly what the expected behavior is, even after spending quite a long time trying to understand and document them.

Copy link
Contributor

Choose a reason for hiding this comment

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

And yes, in that case, the test should be under test/eval/

Copy link
Member

@rocky rocky Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When we test the evaluation of an expression like MatchQ[1+2 a, x_.+y_.*z_Symbol], many processes are involved: the main evaluation loop, the definitions of Plus and Times and their internal implementation, the pattern matching done in the evaluation loop, and eventually, the implementation of matchQ and the pattern matching mechanisms. If we want to know if the pattern matching works as it should, it would be better to use mathics.eval.patterns.match, which does not try to evaluate its arguments, but just implements the pattern matching. I think that this is the right level of testing for these tests.

Ok. If testing Matcher.match() in mathics.eval.patterns.match is a good place to test against, then let's do that and that would be done in a test.eval.pattern module.

I am just saying that if we're going to test temporarily using MatchQ, then testing.core.patterns is not the right test module placement for this.

As written, the test is extending MatchQ tests, even if the motivation for doing this is to verify what's going on at a lower level. I leave to @aravindh-krishnamoorthy whether to rewrite using Matcher.match() inside test.eval.pattern or move the existing tests to test.builtin.testing_expressions.expression_tests and wherever Rules are testing.

Lower levels involve testing whether the functions and methods used to evaluate matching do what they are supposed to. But at this point, there are functions for which I am still unable to say exactly what the expected behavior is, even after spending quite a long time trying to understand and document them.

Then I think it was a mistake to "optimize" what is not well understood nor, as we have seen, not well covered in tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'll always have something that's more interesting to work on (in this case Rubi). So, I want to do it right right now. I'll first try to rewrite the current tests in this PR using Matcher.match() + move them to eval. Only if it turns out to be too cumbersome, I'll fall back to the current ones and move them to expression_tests.

Copy link
Member

@rocky rocky Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your understanding and patience. Sure, it if turns out to be too cumbersome, do as you say.

In fact, the messes in this code have been so great that often I find that I can't fix up a single mess in one go, but have to break things up into little pieces. As you are learning, Pattern matching is one of those big messes.

Down the line, we will use trie-based machining akin to the Golang expreduce (and may try to copy its algorithm). In preparation for that though, having more and better pattern-matching tests in advance will help greatly.

"""

import sys
Expand Down Expand Up @@ -55,9 +55,8 @@ def test_patterns(str_expr, msgs, str_expected, fail_msg):
@pytest.mark.parametrize(
("str_expr", "msgs", "str_expected", "fail_msg"),
[
# Rule
("rule=A[a_.+B[b_.*x_]]->{a,b,x};", None, "Null", None),
# Two default arguments (linear)
("rule=A[a_.+B[b_.*x_]]->{a,b,x};", None, "Null", None),
("A[B[1]] /. rule", None, "{0, 1, 1}", None),
("A[B[x]] /. rule", None, "{0, 1, x}", None),
("A[B[2*x]] /. rule", None, "{0, x, 2}", None),
Expand Down