search

Mathics3 / mathics-core

An open-source Mathematica. This repository contains the Python modules for WL Built-in functions, variables, core primitives, e.g. Symbol, a parser to create Expressions, and an evaluator to execute them.
https://mathics.org
Other
782 stars 48 forks source link

1 / Fix issues with pattern matching for Rubi #1176

Closed aravindh-krishnamoorthy closed 2 days ago

aravindh-krishnamoorthy commented 6 days ago

Overview

In this PR, fixes for pattern matching, esp. those that concern Rubi`, are collected.

Related issues

Failing patterns

(Fixes for items struck out above are moved to the next PR.)

Final review

Tests

Documentation

N/A

rocky commented 6 days ago

For the kinds of problems this fixes, please let's add a test that gets run for this. This way we can ensure no regressions to the code.

Since MatchQ[] is currently in mathics.core.testing_expressions a test case could be added to test/builtin/test_testing_expressions.py Or if you want a more isolated kind of unit test, a new file test/core/test_patterns.py would be created. Thanks.

aravindh-krishnamoorthy commented 6 days ago

For the kinds of problems this fixes, please let's add a test that gets run for this. This way we can ensure no regressions to the code.

Fully agree! I will definitely add them before I finalise this PR.

aravindh-krishnamoorthy commented 4 days ago

Pattern x_Integer + y_Integer z_. for 1 + 2 x (Not directly relevant for Rubi)

Possibly due to this optimization? https://github.com/Mathics3/mathics-core/blob/0d8a8f6e7cd17348870e501f4ce99103772be5df/mathics/core/pattern.py#L783-L796

rocky commented 4 days ago

We need to get it right before we "optimize". So please fix. Thanks.

aravindh-krishnamoorthy commented 4 days ago

We need to get it right before we "optimize". So please fix. Thanks.

Sorry, @rocky, what do you mean? The mentioned optimization is not in the PR but in the mathics-core code.

rocky commented 4 days ago

We need to get it right before we "optimize". So please fix. Thanks.

Sorry, @rocky, what do you mean? The mentioned optimization is not in the PR but in the mathics-core code.

I mean please put in PR to fix add to this PR a fix for the mathics-core code. Thanks.

mmatera commented 4 days ago

Pattern x_Integer + y_Integer z_. for 1 + 2 x (Not directly relevant for Rubi)

Possibly due to this optimization?

https://github.com/Mathics3/mathics-core/blob/0d8a8f6e7cd17348870e501f4ce99103772be5df/mathics/core/pattern.py#L783-L796

If you want to deactivate the optimization, just set leading_blanks=False before the loop.

mmatera commented 4 days ago

Up to here, the changes look reasonable, but it would be nice to see some tests that pass after these changes.

rocky commented 4 days ago

Up to here, the changes look reasonable, but it would be nice to see some tests that pass after these changes.

Let me go further and explain. From my side, it is better to have many smaller fixes and PRS of specific independent bugs than wait until everything is done in several weeks/months and have one huge PR.

It is also useful to add tests after each bug fix is done rather than view this as some sort of separate and independent bullet item or task.

aravindh-krishnamoorthy commented 4 days ago

Indeed, it makes sense to have smaller PRs. I will finalise these with tests soon.

aravindh-krishnamoorthy commented 4 days ago

Pattern x_Integer + y_Integer z_. for 1 + 2 x (Not directly relevant for Rubi) Possibly due to this optimization? https://github.com/Mathics3/mathics-core/blob/0d8a8f6e7cd17348870e501f4ce99103772be5df/mathics/core/pattern.py#L783-L796

If you want to deactivate the optimization, just set leading_blanks=False before the loop.

Perhaps the removal of optionals in this PR can be reverted after we undo the leading_blanks optimization. I'll double-check that in the next PR.

rocky commented 2 days ago

Great - thanks!