ENH: guard against incorrect use of mp.dps?

[mdhaber]

I am a SciPy maintainer, and we frequently use mpmath to compute reference values. (Thank you for your work! It's very helpful.)

A common mistake is for contributors to do, e.g.:

import numpy as np
import mpmath as mp  # should be from mpmath import mp
mp.dps = 50
a, b = mp.mpf(1e-11), mp.mpf(1.001e-11)
print(np.float64(mp.ncdf(b) - mp.ncdf(a)))  # 3.885780586188048e-15

This suffers from catastrophic cancellation just as:

from scipy.special import ndtr
a, b = 1e-11, 1.001e-11
print(ndtr(b) - ndtr(a))  # 3.885780586188048e-15

does, but the fact that mpmath is not working with 50 digits is obscured by the conversion to float. (I've run into other, subtler reasons for not noticing the problem, too.)

Another example in the wild: scipy/scipy#18088 (comment)

I know that this is user error and not a bug in mpmath, but I wonder if it is possible to guard against this. For instance, mpmath doesn't currently have a dps attribute. Perhaps it could be added as a property, and a warning (or error) could be raised if modification is attempted, directing the user to mpmath.mp.dps?

[asmeurer]

I guess the reason it's mpmath.mp.dps instead of mpmath.dps is that you can't define setattr on a module (even after https://peps.python.org/pep-0562/), so to make mpmath.dps = ... work would require monkeypatching the module object (note that dps is a property

mpmath/mpmath/ctx_mp_python.py

Line 621 in 9d7884b

dps = property(lambda ctx: ctx._dps, _set_dps)

). But maybe it can be done in a way that isn't too hacky?

[skirpichev]

#608 seems to be an example of this
and #617

[cbm755]

I tried a bit.

1. property doesn't seem to work for modules.
2. Following PEP 562, I can prevent import mpmath as mp and then mp.dps can be made to raise an AttributeError. But one can still do mp.dps = 64. I can send a PR for this partial solution, but I don't think it helps very much.
3. I read this https://stackoverflow.com/a/48916205 and https://stackoverflow.com/a/2447383 but haven't tried it yet.

[skirpichev]

I guess the reason it's mpmath.mp.dps instead of mpmath.dps is that you can't define setattr on a module ... so to make mpmath.dps = ... work

No. That just will be wrong. There are different contexts and they are totally independent.

I can send a PR for this partial solution, but I don't think it helps very much.

Agreed.

setattr on a module

Probably is only a non-hackish solution to raise exceptions for things like mpmath.dps=123... This will require a PEP, however.

Any other suggestions? Keep in mind also, that the true solution should be a free lunch, i.e. no performance cost (that's about https://stackoverflow.com/questions/2447353/getattr-on-a-module/2447383#2447383, for instance).

UPD: PEP 562 discussion

[cbm755]

From the discussion @skirpichev linked is it possible to hack this:

mpmath.__dict__.__setitem__("dps", 42)

(Is that what mpmath.dps = 42 is doing?) Replace the __dict__ with a custom one that screens for "dps/prec" in setitem. Violates your "free lunch" mantra but maybe only a little.

[cbm755]

Hey! No fair, how come they get readonly attributes but we don't?

>> mpmath.__dict__ = MyHackyDict(mpmath.__dict__)

AttributeError: readonly attribute

[skirpichev]

No fair, how come they get readonly attributes but we don't?

This is magic stuff, coming from the C-part of the CPython world...

Violates your "free lunch" mantra but maybe only a little.

Lets see how much it costs.

FYI: this is how the __getattr__() magic for the module object is implemented.
I'll try to implement a custom tp_setattro (now it fallbacks to the PyObject_GenericSetAttr).

[skirpichev]

Ok, here is a simple implementation: https://github.com/skirpichev/cpython/tree/module-setattr

Test module:

$ cat spam.py 
foo = 1

def __setattr__(name, value):
    if name == 'spam':
        raise AttributeError("Don't spam anymore!")
    globals()[name] = value

CLI session:

>>> import spam
>>> spam.foo
1
>>> spam.bar
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'spam' has no attribute 'bar'
>>> spam.spam
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'spam' has no attribute 'spam'
>>> spam.spam=111
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sk/src/cpython/spam.py", line 5, in __setattr__
    raise AttributeError("Don't spam anymore!")
AttributeError: Don't spam anymore!
>>> spam.bar=2
>>> spam.bar
2

I guess, this is more or less what we want. But maybe the logic should be more close to the __getattr__ support implementation: i.e. if an attribute already exists - we should just run PyObject_GenericSetAttr()...

If it so, let's construct more examples where it could be useful, before proposing this feature on discuss.python.org. BTW, this could be already proposed and rejected...

[WarrenWeckesser]

FWIW: this idea is derived from a code snippet discussed in the rejected PEP 549 that is referred to in PEP 562.

I added the following to mpmath/__init__.py:

import sys
import types


class SetMPMathPrecisionError(RuntimeError):

    def __init__(self, attribute):
        self._attribute = attribute
    
    def __str__(self):
        name = self._attribute
        return (f"cannot set '{name}' on 'mpmath'. Did you mean to "
                f"set '{name}' on 'mpmath.mp'?")


class _MPMathModuleType(types.ModuleType):

    def _set_dps(self, value):
        raise SetMPMathPrecisionError('dps')

    dps = property(fset=_set_dps)

    def _set_prec(self, value):
        raise SetMPMathPrecisionError('prec')

    prec = property(fset=_set_prec)


sys.modules[__name__].__class__ = _MPMathModuleType

Then attempting to set mpmath.dps or mpmath.prec raises an error:

In [1]: import mpmath

In [2]: mpmath.dps = 50
---------------------------------------------------------------------------
SetMPMathPrecisionError                   Traceback (most recent call last)
Cell In [2], line 1
----> 1 mpmath.dps = 50

File ~/py3.10.8/lib/python3.10/site-packages/mpmath/__init__.py:19, in _MPMathModuleType._set_dps(self, value)
     18 def _set_dps(self, value):
---> 19     raise SetMPMathPrecisionError('dps')

SetMPMathPrecisionError: cannot set 'dps' on 'mpmath'. Did you mean to set 'dps' on 'mpmath.mp'?

In [3]: mpmath.prec = 96
---------------------------------------------------------------------------
SetMPMathPrecisionError                   Traceback (most recent call last)
Cell In [3], line 1
----> 1 mpmath.prec = 96

File ~/py3.10.8/lib/python3.10/site-packages/mpmath/__init__.py:24, in _MPMathModuleType._set_prec(self, value)
     23 def _set_prec(self, value):
---> 24     raise SetMPMathPrecisionError('prec')

SetMPMathPrecisionError: cannot set 'prec' on 'mpmath'. Did you mean to set 'prec' on 'mpmath.mp'?

[skirpichev]

@WarrenWeckesser, thanks!

I think this is more slow than the above solution (please test if you will make a PR), but it could work on 3.5+. Probably, we should override getters for dps/prec too.

[oscarbenjamin]

Maybe it would be better if the basic recommended way to set the precision was not just directly setting attributes on the global context objects. If it was something like

from mpmath import mp
mp.set_prec(100)

then you wouldn't have this problem because the module wouldn't have any set_prec attribute.

Ideally you would avoid using the global contexts anyway so like:

from mpmath import make_context
mp = make_context(dps=100)
print(mp.cos(1))

Even for interactive use that's not much less convenient than manipulating the global context:

from mpmath import mp
mp.dps = 100
print(mp.cos(1))

You can currently do this FPContext or MPContext but they don't take any constructor arguments and I think it would be easier for most people to understand if the domain was more explicit perhaps like:

ctx = make_context(domain='real', dps=100)

[skirpichev]

@oscarbenjamin, maybe behave like gmpy2 (see https://gmpy.readthedocs.io/en/latest/contexts.html#contexts on my fork; unfortunately official docs seems to be outdated) does make sense for the mpmath as well? Actually, most functions in the global mpmath namespace are actually methods of the mp context, so we also have some notion of the current context:

>>> from mpmath import iv, mp, sin
>>> sin(1)
mpf('0.8414709848078965')
>>> mp.sin(1)
mpf('0.8414709848078965')
>>> iv.sin(1)
mpi('0.8414709848078965', '0.84147098480789662')
>>> mp.dps=5
>>> sin(1)
mpf('0.84147072')
>>> mp.sin(1)
mpf('0.84147072')
>>> iv.sin(1)
mpi('0.8414709848078965', '0.84147098480789662')
>>> iv.dps=5
>>> iv.sin(1)
mpi('0.84147072', '0.84147167')

Instead it could be:

>>> ctx = get_context()
>>> ctx.dps = 5
>>> set_context(ctx)
>>> sin(1)
...

or

>>> ctx = mp_context(dps=5)  # also fp_context() and so on, maybe with different options
>>> set_context(ctx)
>>> sin(1)
...

This will be a compatibility break, however, and probably we should also implement some solution from proposed above (raise an exception or make mpmath.dps to be an alias of mpmath.mp.dps).

[oscarbenjamin]

This will be a compatibility break, however

I would not suggest breaking compatiblity but just maybe adding a slightly different interface to use contexts and then recommending that in the docs. Ideally the global contexts should not be messed with because if there are say two different scripts/modules/functions simultaneously changing the context settings then they will conflict. Likewise I'm not sure it's threadsafe etc. Even if it is convenient to mess with them something like set_dps is better than plain attribute access.

I think my preferred interface would just be something like (perhaps with better names):

from mpmath import make_context
mp = make_context(dps=100)

This does not set any global object as a new context or change any settings of the existing global context. You then should use mp like mp.cos(1).

It is already a good idea to reference the mp object explicitly to avoid mixing np.cos, math.cos etc. For anything more than interactive or very simple use if you don't want the default context settings you should make your own context anyway. In simple cases it is nice to not worry about global state and thread safety etc so it is nice to be able to do:

from mpmath import mp
mp.dps = 100

However this is not really a much nicer interface for simple interactive use than using make_context as I showed above. Neither is it nicer than just calling set_dps. Where make_context becomes less convenient than a global context is in a complicated script/library with multiple modules that has many functions all needing to access mp.cos etc. However in that more complicated case you definitely should not use the global contexts: at minimum the context should be local to the script/library (even if it is global within the library).

[skirpichev]

I would not suggest breaking compatibility but just maybe adding a slightly different interface to use contexts and then recommending that in the docs.

Sure, but in the end we should switch to that API.

Anyway, a better API, probably, is subject of a different issue. We should fix this one. This approach seems to be working without speed loss. Using that we can:

1. raise exceptions for changing attributes mpmath.dps, mpmath.prec, etc (pretty and so on?). Original suggestion.
2. make mpmath.dps to be an alias of mpmath.mp.dps, ditto for others. IIUIC, this is @asmeurer's suggestion.

With option (1) we could guide users to organize imports as it's suggested in our docs. Option (2) is fully transparent: things will start "just work". This also does make sense as the mpmath.mp is a default context. So, now I'm +0 for 2-nd.

[cbm755]

@WarrenWeckesser can you send a MR? I'd like to try that out.

[cbm755]

Re: what @skirpichev articulated: I'm +1 for "option (1)", and maybe "-epsilon" for "option (2)". My (weak) argument against is that implicitly you'd have to support that in the future. That energy could be put to better use supporting new interfaces along the lines of what @oscarbenjamin and others suggested.

[oscarbenjamin]

My (weak) argument against is that implicitly you'd have to support that in the future

I think this is a strong argument. In my experience there will always be something somewhere that breaks as a result of something like this monkeypatching. Any feature that can only be made to work through monkeypatching the module object commits in some sense to using that monkeypatching forever or breaking compatibility at some future time.

Option 1 is less problematic because a "feature" that only raises an error/warning can more easily be removed in future but I still expect that at some point you would want to remove it.

[WarrenWeckesser]

I agree with @cbm755 and @oscarbenjamin that option 2 is not really a good solution. Setting an attribute of mpmath to change to the behavior of mpmath.mp is not a good API, and is not something that you would want to maintain in the long run.

... can you send a MR? I'd like to try that out.

It would be easy enough to a create a pull request, but I probably won't have time for all the likely follow-up (unit tests, check benchmarks, release note (maybe), fix the unexpected breakage that the change causes (those darn unknown unknowns), etc.), so I don't plan on creating a pull request at the moment. Anyone is welcome to grab that code and modify it as they see fit to create a pull request. If you just want to try it out, it is really as easy as copying that block of code into the beginning of mpsci/__init__.py.

[cbm755]

Thanks @WarrenWeckesser I committed that code in #678 (and learned how to use Co-authored-by: in commit messages)

Comments/reviews/testing welcome.

[asmeurer]

You could make setting mpmath.dps raise a warning or error. So that way we don't commit to the API but it does help people avoid the gotcha.

[skirpichev]

Hmm, overloading __class__ like above reduces speed access for attributes ~2x (see #678).

[oscarbenjamin]

I don't think that there is a particularly good option for preventing attributes being set on a module but what this issue shows is that perhaps an interface based on setting attributes is error-prone. In Python attributes can be set on modules and that is a basic language feature. It would be nice if there was a good way to prevent that but there isn't.

It would be better to think about how to have a less error prone interface rather than trying to find some way to make setting an attribute cause an error. The simplest option is just to have a method/function set_dps and make sure that all docs suggest that over direct attribute access.

[skirpichev]

but what this issue shows is that perhaps an interface based on setting attributes is error-prone

I doubt:) It just shows that people are trying to follow numpy-like import conventions (import numpy as np) and this clashes with the context class name in the mpmath. And/or they don't read the docs.

I don't think there is something wrong with the current context objects API as long as these objects are mutable. ctx.prec = 42 is more pythonic than ctx.set_prec(42) (the later case I would pick up for immutable objects, like this: newctx = ctx.set_prec(42)).

It would be nice if there was a good way to prevent that but there isn't.

There is, at the cost of some slowdown (real, but not important, IMHO). But if you think it's an important thing - then we could consider adding a different API just for this issue.

[oscarbenjamin]

Python as a language allows doing lots of things that are sometimes convenient but realistically should be avoided in widely used packages and I think that monkey-patching the top-level module of mpmath falls into that category.

The problem that this is trying to solve is that some users might use mpmath incorrectly. That problem stems from the fact that mpmath suggests for users to control precision by setting an attribute on an object. Setting attributes is an error prone interface in Python because most objects can have any attribute set on them without any error e.g.:

>>> from mpmath import cos
>>> cos.dps = 100
>>> cos(1)
mpf('0.54030230586813977')

The problem also stems from the fact that mpmath has an odd interface partly by suggesting from mpmath import mp which is different from the more common import numpy as np. It is also confusing though because both mpmath and mpmath.mp can be used interchangeably in other situations:

>>> import mpmath
>>> from mpmath import mp
>>> mpmath.cos(1)
mpf('0.54030230586813977')
>>> mp.cos(1)
mpf('0.54030230586813977')

If mpmath were clearer about which of these objects is supposed to be used then the problem would not arise.

Lastly the problem also stems from the fact that mpmath's interface encourages users to access it through an object that represents mutable global state which is also error-prone because global state is not checked for errors.

There are lots of ways that mpmath could provide a slightly different interface that would implicitly solve the problem that this issue describes but also other problems like avoiding the use of mutable global state in the first place.

[asmeurer]

Another option: set mpmath.dps = None or some other sentinel value, and check somewhere (e.g., somewhere in the mpf class or somewhere in the Context class) if it is set to something other than None, and if so, issue a warning/error.

[skirpichev]

set mpmath.dps = None or some other sentinel value, and check somewhere

I would expect that this will be more slow than #678: this should be tested in every context's function (and for several attributes). And, probably, much more fragile as well.