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

Earlier initialization of patterns #1103

Merged
merged 3 commits into from
Sep 30, 2024
Merged

Earlier initialization of patterns #1103

merged 3 commits into from
Sep 30, 2024

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Sep 28, 2024

When a ExpressionPattern is created, the way in which the matching with expressions is determined in part by the attributes of its head. For example, if S is Orderless, the match method of the pattern
S[a_, v__Integer] should check for different orders of the arguments.

Also, the match against patterns like DirectedInfinity[1] can be determined using the faster sameQ method, instead of using all the machinery for looking for named patterns inside.

This PR introduces some modifications in the way Pattern objects are created, to have earlier access to the attributes, and to determine if the pattern is a "Literal"

@rocky
Copy link
Member

rocky commented Sep 29, 2024

Please let me know when it should be looked at on a cursory level. Basically, there was already Literal work planned and there was also a total revamp of pattern matching. The Go code that was mentioned elsewhere should be looked at and considered.

The little I looked at looked more like underbrush removal which we find ourselves doing too often. But some consideration should be given to how this might fit into the bigger picture of planned work. I don't think this conflicts, but I'd like to think about it and check and/or discuss it.

@mmatera
Copy link
Contributor Author

mmatera commented Sep 29, 2024

Please let me know when it should be looked at on a cursory level. Basically, there was already Literal work planned and there was also a total revamp of pattern matching.

Actually, here it is not so much: just make PatternBase.create more aware about the attributes of the pattern it has to create, and if the pattern is just a "Literal" ("Verbatim"?) expression, just tag the Pattern object to use sameQ instead of something more sophisticated that tries to track named patterns, or Orderless expressions.

The question here is if it makes sense to add specific classes for these "Literal" patterns, or just keep a single ExpressionPattern class, that changes its methods according to the attributes and tags.

@rocky
Copy link
Member

rocky commented Sep 30, 2024

Actually, here it is not so much: just make PatternBase.create more aware about the attributes of the pattern it has to create, and if the pattern is just a "Literal" ("Verbatim"?) expression, just tag the Pattern object to use sameQ instead of something more sophisticated that tries to track named patterns, or Orderless expressions.

Ah ok. I just pulled the branch and looked it over thinking about things. This is not disruptive. Adding a property or attribute indicating whether a pattern is literal is good. And being able to use SameQ to test pattern equality is also good.

I am sorry for the delay in getting to this.

The question here is if it makes sense to add specific classes for these "Literal" patterns, or just keep a single ExpressionPattern class, that changes its methods according to the attributes and tags.

Right now, I don't see a need for a separate class like there is for ListExpression.

I am fine with merging this in now as is.

@mmatera mmatera marked this pull request as ready for review September 30, 2024 15:02
@mmatera
Copy link
Contributor Author

mmatera commented Sep 30, 2024

Actually, here it is not so much: just make PatternBase.create more aware about the attributes of the pattern it has to create, and if the pattern is just a "Literal" ("Verbatim"?) expression, just tag the Pattern object to use sameQ instead of something more sophisticated that tries to track named patterns, or Orderless expressions.

Ah ok. I just pulled the branch and looked it over thinking about things. This is not disruptive. Adding a property or attribute indicating whether a pattern is literal is good. And being able to use SameQ to test pattern equality is also good.

I am sorry for the delay in getting to this.

The question here is if it makes sense to add specific classes for these "Literal" patterns, or just keep a single ExpressionPattern class, that changes its methods according to the attributes and tags.

Right now, I don't see a need for a separate class like there is for ListExpression.

I am fine with merging this in now as is.

OK, in that case, let's merge and iterate

@@ -92,6 +92,11 @@ class BasePattern(ABC):

expr: BaseElement

# this attribute allows for a faster match algorithm based on sameq.
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean sameQ ?

@mmatera mmatera merged commit 6aadef6 into master Sep 30, 2024
11 checks passed
@mmatera mmatera deleted the literal_patterns branch September 30, 2024 15:03
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