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

Reduce RealNumberQ in builtin.image.basic and ... #823

Merged
merged 3 commits into from
Mar 23, 2023
Merged

Conversation

rocky
Copy link
Member

@rocky rocky commented Mar 23, 2023

Disallow Python 3.6
move is_integer_rational_or_real to mathics.core.atoms

Disallow Python 3.6
move is_integer_rational_or_real to mathics.core.atoms
rules = {
"Blur[image_Image]": "Blur[image, 2]",
"Blur[image_Image, r_?RealNumberQ]": "ImageConvolve[image, BoxMatrix[r] / Total[Flatten[BoxMatrix[r]]]]",
"Blur[image_Image, r_?RealNumberQ]": (
Copy link
Member Author

@rocky rocky Mar 23, 2023

Choose a reason for hiding this comment

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

I haven't been able to figure out a replacement for matchpattern _?RealNumberQ pattern here.

Copy link
Contributor

Choose a reason for hiding this comment

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

One possibility would be r:(_Integer|_Real|_Rational). On the other hand, we can keep RealNumberQ but put it in a private context.

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 another one: r_/;(NumberQ[r] && (Im[r]==0)). Also we can just ask for NumberQ, and then in the "body" check r has a nonzero imaginary part and send a message. Then pass BoxMatrix the real part.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, there is another one: r_?NumberQ and then define another rule specific for Complex numbers, that just show the warning.

In any case, I wonder why there is no keyword in WL that allows having the behavior we obtain when we implement the rule as a method, and the method returns (Python's) Null.

Copy link
Member Author

@rocky rocky Mar 23, 2023

Choose a reason for hiding this comment

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

One thing I am coming to understand is that this rewrite-rule approach generally leads to different behavior than WMA.

First, when there is no match, Mathics3 silently fails whereas in WMA a type error message is given.

Second, when there is an error detected, a rewrite may have already occurred leading to a result that was different from the original input given. For example If I write;

ImageAdjust[x, "a"]

By the time the message occurs that "a" is invalid, This has already been rewritten to ImageAdjust[x, {"a", 0, 1}] and this is confusing. More confusing with "Blur" since that function name was replaced altogether.

While on the one hand it is nice that there was an attempt to use more Mathics3 as opposed to Python, on the other hand it leads to less WMA conformance, and more user confusion. (And currently slower too since our pattern matching is in great need of speedup).

So again, to me this is another example of "too clever by a half".

pattern rule thanks to mmatera
@rocky
Copy link
Member Author

rocky commented Mar 23, 2023

Merging and iterating...

@rocky rocky merged commit 24be2a8 into master Mar 23, 2023
@rocky rocky deleted the reduce-RealNumberQ-3 branch March 23, 2023 16:16
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