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

Moving private doctests to pytests. Fix the compatibility issue 0. I-> 0. #895

Merged
merged 3 commits into from
Aug 2, 2023

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Aug 1, 2023

In this PR, I start to move some private doctests to Pytest. I also propose some extra related tests and check that the expected behavior matches with obtained in WMA.
From this, it came up that we are handling wrong how 0. I is evaluated: In master, it is evaluated to 0., but in WMA, it is evaluated to Complex[0.`, 0.`].
To match this behavior, I had to modify from_mpmath, and deactivate its lru_cache. The problem now with lru_cache is that mpmath.mpf(0.) produces the same hash than mpmath.mpc(0., 0.), which looks wrong.

@@ -999,7 +875,7 @@ class RealNumberQ(Test):
>> RealNumberQ[0 * I]
= True
>> RealNumberQ[0.0 * I]
= True
= False
Copy link
Member

Choose a reason for hiding this comment

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

Recall there is no RealNumberQ in WMA.

Eventually I'd like to remove RealNumberQ from Mathics3. It would be okay to have an eval_RealNumberQ though. Can we revise this revised to use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have this as a hack to put it in a rule. I agree, we should remove this at some point. By now, I just adjusted the test according to the actual behavior of 0. I

Copy link
Member

@rocky rocky Aug 2, 2023

Choose a reason for hiding this comment

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

Ok - but please consider doing this before contemplating anything that includes bigger picture design work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was investigating a little bit about this, and I found this discussion:https://mathematica.stackexchange.com/questions/18647/how-to-check-if-an-expression-is-a-real-valued-number

So, in WMA, there is an internal symbol that makes exactly what RealNumberQ does, but with a slightly different name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And actually, in the last version, RealValuedNumberQ is a system symbol.

@@ -18,7 +18,21 @@
from mathics.core.systemsymbols import SymbolIndeterminate


@lru_cache(maxsize=1024)
# Another issue with the lru_cache: for mpmath, mpmath.mpc(0.,0.) and
Copy link
Member

Choose a reason for hiding this comment

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

The problem is putting the lru_cache decorator any any function that has an optional parameter.

If we rewrite such function to call another function that does not use optional parameters, but has all of the parameters filled out and put the lru decorator there, I don't think there will be problem.

@@ -42,8 +56,8 @@ def from_mpmath(
# HACK: use str here to prevent loss of precision
return PrecisionReal(sympy.Float(str(value), precision=precision - 1))
elif isinstance(value, mpmath.mpc):
if value.imag == 0.0:
return from_mpmath(value.real, precision=precision)
# if value.imag == 0.0:
Copy link
Member

Choose a reason for hiding this comment

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

If this is going to stay as a comment, then it would be helpful to explain why this does not work. For example, list an example where the evaluation does the wrong thing.

Copy link
Contributor Author

@mmatera mmatera Aug 2, 2023

Choose a reason for hiding this comment

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

I asked in the mpmath project mpmath/mpmath#714, and indeed is the expected behavior for mpmath and Python.
In any case, the problem with the lru_cache is fixed by passing the parameter typed=True.

Copy link
Member

Choose a reason for hiding this comment

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

In another PR then let's reinstate the lru_cache adding the typed=True parameter. This is a better solution than the one proposed before. But there a other places in the code where lru_cache was used, and the split function technique was used. That then should be adjusted not to split the function and use typed=True instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is weird: I made the change, commit and push it to this branch, but for some reason, it never appeared here. I put this in another PR with the missing lines...

@rocky
Copy link
Member

rocky commented Aug 2, 2023

To match this behavior, I had to modify from_mpmath, and deactivate its lru_cache. The problem now with lru_cache is that mpmath.mpf(0.) produces the same hash than mpmath.mpc(0., 0.), which looks wrong.

See comments in code for how this can probably be addressed keeping lru_cache.

Overall, this looks good. Thanks for undertaking this!

LGTM - but see the other small comments.

@mmatera mmatera merged commit eaace9f into master Aug 2, 2023
11 checks passed
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