More compatibility improvements in Accuracy and Precision. Adding tests. #769

mmatera · 2023-01-30T02:34:30Z

This PR is another fix coming from issues found when I was working on #766. In particular, the way in which Mathics handles Precision and Accuracy for real numbers with 0. nominal value. Also, a bug that prevents parsing Real numbers with fixed accuracy was fixed.

rocky · 2023-01-30T04:06:18Z

The thrust or idea behind this is good. Update: the more I look at this code the more I like it.

However, I would like to spend some time understanding things in detail. I want to make sure I understand things - that may take a little while.

mathics/eval/numbers.py

rocky · 2023-01-30T08:46:08Z

CHANGES.rst

@@ -92,9 +94,10 @@ Bugs

 #. Units and Quantities were sometimes failing. Also they were omitted from documentation.
 #. Better handling of ``Infinite`` quantities.
-#. Fix ``Precision`` compatibility with WMA.
+#. Fix ``Precision`` and ``Accuracy``compatibility with WMA. In particular, ``Precision[0.]`` and ``Accuracy[0.]``
+#. Accuracy in numbers usint the notation ``` n.nnn``acc ```  now is properly handled.


usint -> using

A small thing about using the word "Fix". Does it mean that all the problems with Precision have really been corrected or is it that the previous problems have been reduced, that is, that Precision has been improved?

The section on "Precision" says

This is rather a proof-of-concept than a full implementation

Is that still the case after this PR?

The section on "Accuracy" says:

Notice that the value is not exactly equal to the [value] obtained in WMA

Is this still accurate?

In all the cases I have tested, I get the same result as in WMA (up to small numeric differences due to the mpmath implementation).

My style is to not exaggerate. This is then "more properly handle" as the next change lists, or "improve" than "fix".

The differences I found are in this order:

'Accuracy[0.32`30*^30]

WMA: 0.4948500216800894
Mathics: 0.49485002168009373

For Complex, there could be more differences:

Precision[12`20*^30+13`15*^24]
WMA: 19.9553
Mathics: 20.

Accuracy[12`20*^30+13`15*^24]
WMA: -11.1239
Mathics: -11.0792

Probably in this case the normalization is different. In any case, differences are of at most ~O(1/10). So, all the currently detected issues are fixed. But OK, I can rephrase it.

Thanks for performing the experiments. It is nice to know this. I think we should document this somewhere so we don't lose this useful information.

rocky · 2023-01-30T08:53:40Z

mathics/builtin/atomic/numbers.py

+     >> Accuracy[0.``2]
+      = 2.
+
+    For 0.`, the accuracy is the 


is the -> is:

Colon at the end after "is" please.

rocky · 2023-01-30T08:57:01Z

mathics/builtin/atomic/numbers.py

+    >> Precision[0.]
+     = MachinePrecision
+
+    On the other hand, for a Precision Real with fixed accuracy,


Add a backslash after "fixed accuracy,"

rocky · 2023-01-30T08:57:42Z

mathics/builtin/atomic/numbers.py

+     = True
+
+    The case of `0.` values is special. Following WMA, in a Machine Real representation, the precision is
+    set to $MachinePrecision:


$MachinePrecision -> '$MachinePrecision'

Please add "\" to the end of the line ("the precision is").

rocky · 2023-01-30T08:57:56Z

mathics/builtin/atomic/numbers.py

@@ -948,6 +934,23 @@ class Precision(Builtin):
    >> Precision[{{1, 1.`},{1.`5, 1.`10}}]
     = 5.

+    For non-zero Real values, it holds in general


general -> general:

mathics/builtin/numbers/constants.py

rocky · 2023-01-30T09:03:01Z

mathics/core/number.py

+MAX_MACHINE_NUMBER = float_info.max
+ZERO_MACHINE_ACCURACY = -mpmath.log(MIN_MACHINE_NUMBER, 10.0) + MACHINE_PRECISION_VALUE
+
+# backward compatibility


Is this necessary? This release we are bumping the major number. If so, suggest possibly in the comment when we can remove this. (I would find it hard to believe that anyone other than ourselves is making use of this.)

I leave this commentary because that variable appears in several modules. I can make the change, but then this PR would become larger. Also, I wondered if it was worth making the change. Thoughts?

I think it worth making the change. If you want to do in another PR, that's okay.

Then, I will do that in the following PR.

mathics/eval/numbers.py

rocky · 2023-01-30T09:10:37Z

mathics/builtin/numbers/constants.py

+          as a normalized machine number in the system.
+    </dl>
+
+    MachinePrecision minus the Log base 10 of this number is the 


MachinePrecision -> 'MachinePrecision'

I am not seeing quotes around 'MachinePrecision' on line 623. Maybe a commit that wasn't pushed?

mathics/eval/numbers.py

rocky · 2023-01-30T09:20:14Z

test/builtin/atomic/test_numbers.py

-        # ("Accuracy[-0.000000000000000000000000000000000000]", "36."),
-        ("1.0000000000000000 // Accuracy", "15."),
-        ("1.00000000000000000 // Accuracy", "17."),
+@pytest.mark.parametrize(


I think Accuracy is based on computer hardware characteristics, right? If so we should add a pytest.mark.skipif when those hardware characteristics are not met.

I used the native sys.float_info, so it would adjust to each architecture. If we discover one architecture where it fails, then we can handle it.

Doesn't accuracy depend on MACHINE_PRECISION_VALUE and MIN_MACHINE_NUMBER? and those depend of mpmath values and you say that mpmath varies?

I am okay with leaving out pytest.mark.skipif provided there is an assertion on these underlying values.

And then that way we will know that things have changed.

Recall we had a recent situation with reading bytes from a file that changed due to the endian-ness of the machine.

I would feel differently if we were testing regularly using different CPU architectures and 32-bit machines versus 64-bit OS setups and so on. But we are not.

One other thing here. There should be unit test precisely for the underlying assumptions that are in effect.

One purpose of unit tests is exactly this. (There are other test like functional and integration tests). Our tests tend to be on the heavier side - integration and functional rather than on the smaller lighter, more precise unit tests.

Doesn't accuracy depend on MACHINE_PRECISION_VALUE and MIN_MACHINE_NUMBER? and those depend of mpmath values and you say that mpmath varies?

MACHINE_PRECISION_VALUE and MIN_MACHINE_NUMBER depends on numbers coming from the sys module, so it would work in any platform that runs Python. What should we catch?

I am okay with leaving out pytest.mark.skipif provided there is an assertion on these underlying values.

And then that way we will know that things have changed.

Recall we had a recent situation with reading bytes from a file that changed due to the endian-ness of the machine.

I would feel differently if we were testing regularly using different CPU architectures and 32-bit machines versus 64-bit OS setups and so on. But we are not.

Ah! Now I understand. What I think would be better is to change the tests in a way that the connection with these numbers be reinforced.

Yes, that is even better. And if you are up for it, go for it!

Maybe you can find a 32-bit OS around and see what values you get or try a Python made from a 32-bit C compiler. Or if there are 128-bit architectures or 128-bt C compilers, see what those do.

There might be github workflows CI images that have these properties of 32-bitness or 32-bitness compilers (or other than 64-bitness) and we could run a test using that image.

And the same thing along the lines of different mpc versions.

It occurs to me that is you feel confident in knowing how what MACHINE_EPSILON, etc change things, then in a test you could artificially set these to different values and make the Precision and Accuracy tests.

It would however be nice to know what alternate values these possibly can take on.

I think that with the new changes, the tests are robust across different platforms. The only thing that could go wrong is that certain platform does not have support for floating point numbers, but I guess that in such a case everything gets broken.

mathics/builtin/atomic/numbers.py

rocky · 2023-01-30T09:29:19Z

mathics/builtin/numbers/constants.py

+    name = "$MinMachineNumber"
+    summary_text = "smallest normalized positive machine number"
+
+    def evaluate(self, evaluation) -> MachineReal:


In new code I have been adding a type annotation for evaluation: evaluation: Evaluation

rocky · 2023-01-30T14:18:05Z

@mmatera Branch merged with master because I can't run Django using this and the current master.

rocky · 2023-01-30T14:23:20Z

mathics/builtin/atomic/numbers.py


-    $$Accuracy[z] == Precision[z] + Log[z]$$
+    Accuracy[$z$] == Precision[$z$] + Log[$z$]


Single quotes around 'Accuracy', 'Precision', and 'Log' please.

rocky · 2023-01-30T14:27:55Z

mathics/builtin/atomic/numbers.py

@@ -183,11 +187,17 @@ class Accuracy(Builtin):
     = Infinity

    >> Accuracy[F[1.3, Pi, A]]
-     = 14.8861
+     = 15.8406


If this can vary, the please let's use '...' and make precise tests as pytests where we also have assertions or checks on the underlying factors that make this so.

As you know with the MLMath stuff, I am not a fan of fragile tests because they fail unexpectedly at a time when you really don't want to start dealing with the failure.

In this case, the "fragility" on the test allows for checking the behavior. But maybe I could surround it with Round.

You could, but this is the wrong philosophy: the primary purpose of these tests is to enlighten users as to what this Builtin does; testing is secondary. So complicating an example to make testing is not the right philosophy.

If you want a test, the add this as a pytest where you can get as elaborate as you want, like within so many digits. But better here is to also add checks for the fundamental properties that the this calculation is based on. That way when that assumption breaks for whatever reason, we have pinpointed at a low level what the problem is rather than have to recall or look around in the code what Accuracy assumes that has been violated.

rocky · 2023-01-30T14:33:20Z

mathics/builtin/atomic/numbers.py

@@ -927,8 +914,8 @@ class Precision(Builtin):
      <dt>'Precision[$expr$]'
      <dd>examines the number of significant digits of $expr$.
    </dl>
-
-    <i>This is rather a proof-of-concept than a full implementation.</i>
+    <i>Notice that the result could be slighly different than the obtained


backslash after "obtained" please. For myself I go into Django and look at the rendered output. it can help catching things like this.

more doc adjustments

rocky · 2023-01-30T18:31:42Z

mathics/builtin/atomic/numbers.py

@@ -195,7 +195,7 @@ class Accuracy(Builtin):
     >> Accuracy[0.``2]
      = 2.

-    For 0.`, the accuracy is
+    For 0.`, the accuracy satisfies


ok, but please add a colon after "satisfies" this what we do everywhere (and it happens to be the correct punctuation).

rocky · 2023-01-30T18:32:15Z

test/builtin/atomic/test_numbers.py

-        ("0.00`", "323.607"),
-        ("0.00`2", "323.607"),
-        ("0.00`20", "323.607"),
+        ("0.", ZERO_MACHINE_ACCURACY_STR),


This is actually, more readable. Thanks.

rocky · 2023-01-30T18:33:34Z

test/builtin/atomic/test_numbers.py

-        (" 0.4 + 2.4 I", "15.5744"),
+        # For some reason, the following test
+        # would fail in WMA
+        ("1. I", "Accuracy[1.]"),


Again this and the below are more understandable. Thanks. (And again this is shows how pytests are very different from tests that happen to get run in documentation.)

rocky · 2023-01-30T18:35:05Z

LGTM and seems useful. Thanks. There may be some small lingering punctuation things, but those can be addressed any time.

mmatera added 4 commits January 29, 2023 23:31

more compatibility improvements in Accuracy and Precision. Adding tests.

1d4798c

adding documentation and tests

83fdaf6

CHANGES.rst and manifesf

d857428

Update constants.py

4992107

rocky reviewed Jan 30, 2023

View reviewed changes

mathics/eval/numbers.py Outdated Show resolved Hide resolved

rocky reviewed Jan 30, 2023

View reviewed changes

mathics/builtin/numbers/constants.py Show resolved Hide resolved

rocky reviewed Jan 30, 2023

View reviewed changes

mathics/eval/numbers.py Outdated Show resolved Hide resolved

rocky reviewed Jan 30, 2023

View reviewed changes

mathics/eval/numbers.py Outdated Show resolved Hide resolved

rocky reviewed Jan 30, 2023

View reviewed changes

mathics/eval/numbers.py Show resolved Hide resolved

rocky reviewed Jan 30, 2023

View reviewed changes

mathics/builtin/atomic/numbers.py Outdated Show resolved Hide resolved

rocky reviewed Jan 30, 2023

View reviewed changes

eval_accuracy->eval_accuracy and eval_precision-> eval_Precision

b2024ef

rocky reviewed Jan 30, 2023

View reviewed changes

documentation adjustments

db76dc1

mmatera force-pushed the more_on_accuracy_and_prec branch 2 times, most recently from 31ae1b1 to 93a7703 Compare January 30, 2023 18:04

make the tests more robusts

f88e6e5

more doc adjustments

mmatera force-pushed the more_on_accuracy_and_prec branch from 93a7703 to f88e6e5 Compare January 30, 2023 18:05

rocky reviewed Jan 30, 2023

View reviewed changes

Update numbers.py

c86cd7c

mmatera merged commit 40a446d into master Jan 30, 2023

mmatera deleted the more_on_accuracy_and_prec branch January 30, 2023 19:38


		$$Accuracy[z] == Precision[z] + Log[z]$$
		Accuracy[$z$] == Precision[$z$] + Log[$z$]

More compatibility improvements in Accuracy and Precision. Adding tests. #769

More compatibility improvements in Accuracy and Precision. Adding tests. #769

Conversation

mmatera commented Jan 30, 2023

rocky commented Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky commented Jan 30, 2023

rocky commented Jan 30, 2023 •

edited

Loading

rocky Jan 30, 2023 •

edited

Loading

rocky Jan 30, 2023 •

edited

Loading

rocky Jan 30, 2023 •

edited

Loading