prevent erroneous setting of dps/prec on mpmath module

[cbm755]

What we really want for Issue #657 is an exception on write access. Instead here is a partial implementation for read access. Not even close to a solution sadly.

update: exceptions on write access to .dps and .prec in mpmath module.

---

Todo list

• depends on Drop support for old py3 versions (<3.8) #663
• release notes?
• test
• any performance concerns?

potential changelog entry

My feeling is this maybe doesn't need a changelog, but just in case here's one:

Bug fixes:

  * Prevent accidentally setting `dps`/`prec` on the mpmath module (Colin Macdonald, Sergey Kirpichev, Warren Weckesser)

Closes #657

[skirpichev]

I doubt this will fix the issue. Misunderstanding starts with setting dps/prec attributes...

[cbm755]

This really doesn't work very well, not sure I understand why:

>> import mpmath as mp
>> mp.dps
AttributeException(...)
>> mp.dps = 64
>> mp.dps
64

eh? I was expecting an exception. So unless I've done this wrong PEP 562 really doesn't offer much control here.

[skirpichev]

This really doesn't work very well, not sure I understand why:

>> import mpmath as mp
>> mp.dps
AttributeException(...)
>> mp.dps = 64
>> mp.dps
64

eh? I was expecting an exception.

Unfortunately, this is not like the property attribute on classes. If 'dps' exists in the namespace of the module - the __getattr__() helper will not be invoked.

So unless I've done this wrong

Certainly not.

[skirpichev]

It seems, release notes (CHANGES) update happens just before release. Probably, it's a good idea to automate this and keep things more up to date. But don't worry here.

Yes, lets add tests. Shouldn't be hard.

I don't think there is any performance penalty. @oscarbenjamin?

UPD: And, BTW, this fix #657

[skirpichev]

Well, I see measurable (~2x) speed regression for attribute access. Not sure if this is significant for the mpmath, however.

An example:

$ cat b.py 
x = 1
$ python -m timeit -r11 -s 'import b' 'b.x'
5000000 loops, best of 11: 48.8 nsec per loop
$ cat c.py 
import sys, types
x = 1
class _Foo(types.ModuleType): pass
sys.modules[__name__].__class__ = _Foo
del sys, types
$ python -m timeit -r11 -s 'import c' 'c.x'
2000000 loops, best of 11: 131 nsec per loop

No speed regression for this scenario on my branch with __setattr__() support, of course:

$ cat d.py 
x = 1
def __setattr__(name, value):
    if name == 'spam':
        raise AttributeError
    globals()[name] = value
$ python -m timeit -r11 -s 'import d' 'd.x'
5000000 loops, best of 11: 48.8 nsec per loop

[cbm755]

I don't know if this is significant here or not... I figure it potentially matters when calling frunctions from mpmath in a loop:

e.py:

import mpmath

def test1():
    f = mpmath.sin
    for i in range(0, 1000):
        f(i / 10)

def test2():
    for i in range(0, 1000):
        mpmath.sin(i / 10)

$ python -m timeit -r11 -s 'from e import test1' 'test1()'
100 loops, best of 11: 3.21 msec per loop
$ python -m timeit -r11 -s 'from e import test2' 'test2()'
100 loops, best of 11: 3.25 msec per loop

So its insignficant there, compared to the computation cost.

Of course, if I remove the computation, then I see the same results as @skirpichev:

import mpmath

def test1():
    f = mpmath.sin
    for i in range(0, 1000):
        f

def test2():
    for i in range(0, 1000):
        mpmath.sin

$ python -m timeit -r11 -s 'from e2 import test1' 'test1()'
50000 loops, best of 11: 8.86 usec per loop
$ python -m timeit -r11 -s 'from e2 import test2' 'test2()'
10000 loops, best of 11: 22.6 usec per loop

So is there some situation where one would tight-loop access to an attribute of mpmath without doing any real work in the loop?

Even if we cannot think of a "real life" example here, it definitely sounds like a point to make if you submit a PEP!

[cbm755]

Here's an example, only one letter different sign instead of sin, which is much cheaper:

import mpmath

def test1():
    f = mpmath.sign
    a = 0
    for i in range(0, 1000):
        a += f(i)

def test2():
    a = 0
    for i in range(0, 1000):
        a += mpmath.sign(i)

$ python -m timeit -r11 -s 'from e import test1' 'test1()'
50000 loops, best of 11: 9.11 usec per loop
$ python -m timeit -r11 -s 'from e import test2' 'test2()'
10000 loops, best of 11: 22.2 usec per loop

And we see the slow down in a vaguely real-life example...

[skirpichev]

So is there some situation where one would tight-loop access to an attribute of mpmath without doing any real work in the loop?

Something like type(foo) is mpmath.mpf or isinstance(foo, mpmath.mpf).

an example, only one letter different sign instead of sin, which is much cheaper ... And we see the slow down in a vaguely real-life example...

Of course, all this can be workarounded by using test1() layout (or from mpmath import sign as f) instead of test2()... So, I'm still in favor of merging this. Let's wait few days in case someone else will review this.

it definitely sounds like a point to make if you submit a PEP!

Unfortunately, I don't see too much usage examples for this "feature". And mpmath's case seems to be very special. What else? To prevent mutations of the module __dict__, e.g. mpmath.sin="spam"?:) On another hand, PEP 562 has not too much as well... (BTW, I don't see where stuff from that PEP was documented.)

[oscarbenjamin]

I think that replacing the module in sys.modules like this is a bad idea. It is messing around with things that should not really be messed with and will cause something to break. I am certain that this change would end up being regretted (this looks like precisely the kind of thing that I would want to remove from a codebase if I saw it).

Being able to define __setattr__ on a module would be a useful change to the Python language. In the absence of a properly supported way to prevent attributes being set on a module though it is better just to accept that this can happen even if it is unfortunate.

It would be better to focus on designing an interface for precision control that is less prone to mistakes and misunderstanding.

[skirpichev]

will cause something to break.

Could you provide an example? We can't reject pr only on the ground of unspecified potential problems.

Being able to define __setattr__ on a module would be a useful change to the Python language.

I doubt it will be accepted. (As I said above, there are no good practical examples of why it could be helpful.) But if it will - we can switch the present workaround to that solution.

In the absence of a properly supported way to prevent attributes being set on a module

Actually, Python does support customizing module attribute access, it is only slow. The recommended way in the CPython docs is exactly how things are implemented in this pr. UPD, FYI: python/cpython#67175

[skirpichev]

FYI: https://discuss.python.org/t/extend-pep-562-with-setattr-for-modules/25506

[skirpichev]

@fredrik-johansson, how this solution looks for you?

[skirpichev]

I think this could be merged as-is, I will do this on monday. Unfortunately, from the discussion thread it seems for me, that proposing a PEP for __setattr__ helper doesn't make sense.

1. There is some scenarios where measurable code slowdown exits, but I doubt this will pose real problems for someone. And there are simple workarounds.
2. @oscarbenjamin, if you have any clue on what this may "break" - please show us. BTW, similar replacing of the module in sys.modules was used for a long time in the scipy (FFT submodule).
3. @asmeurer, I would appreciate your opinion.

[oscarbenjamin]

2. if you have any clue on what this may "break" - please show us

I don't have any specific examples. My experience is that when you do something like this it seems like you can reason about whether it should work but then something somewhere else turns out to have made some assumptions that will be broken as a result of the change.

I think it would be better to reflect on whether setting a global dps attribute is actually a good interface in the first place (I don't think it is) rather than making the change here to give an error for someone who sets the attribute on the wrong object. After that I would consider whether the docs can be improved so that the mistake is less likely. After that I would just do nothing. It isn't ultimately necessary to do anything in response to gh-657: sometimes an issue is just an issue but does not need to be "fixed".

[cbm755]

Perhaps we should just leave this open, then see if such an alternative interface as @oscarbenjamin describes) can be developed. If no such alternative materializes then we can revisit this, say in 6 months or so.

To be concrete: if the docs in the dev tree are are still recommending mp.dps = 64 in 2024, then I suggest merging this.

[skirpichev]

I don't have any specific examples.

Good news. Then I don't see technical objections: this pr does very tiny and fine grained customization of the module behavior, exactly on the way the official docs recommends.

I think it would be better to reflect on whether setting a global dps attribute is actually a good interface in the first place (I don't think it is)

I think we all agreed that the context management could be better. But this is much more complex change (and probably with API breakage).

It isn't ultimately necessary to do anything in response to #657

This is a common gotcha for mpmath users (2-3 duplicates of this issue were mentioned in the issue thread), that's why I would like to include some fix in the next (minor?) release.

Alternatively, we can add something to the docs (as a note/warning or a dedicated section), but this solution seems much better for me: it just forbids wrong scenarios of altering context attributes.

[asmeurer]

I think it's worth at least trying out #657 (comment). It's much less invasive. I think if we put the check in the right place it won't be a major performance issue.

[skirpichev]

To be concrete: if the docs in the dev tree are are still recommending mp.dps = 64 in 2024, then I suggest merging this.

Ok.

[oscarbenjamin]

if the docs in the dev tree are are still recommending mp.dps = 64 in 2024, then I suggest merging this.

I don't see any reason to wait until 2024: if you're going to do it then just do it now. Personally I would not do this now or later because it would be better to fix the interface design rather than add hacky workarounds to make the current interface slightly less confusing. Especially there should be no noticeable performance penalty for users who are using mpmath correctly just to provide better error messages to someone who uses it incorrectly.

It would be better to have an interface that does not depend on either attribute access or setting global variables at all and the docs could easily recommend that:

from mpmath import make_context
mp = make_context(real=True, dps=100)

Note this from the README:

4. Known problems

Mpmath is a work in progress. Major issues include:
...

• The interface for switching precision and rounding is not finalized. The current method is not threadsafe.

I suggest that we think about what is (or could be) a good recommended way for users to use mpmath in the first instance and then consider what to recommend in the docs. Once that is done we might find that this issue is just not an issue any more.

[skirpichev]

it would be better to fix the interface design

I've opened an issue #683

Especially there should be no noticeable performance penalty

I don't think there is a practical performance penalty.

at least trying out #657 (comment). It's much less invasive. I think if we put the check in the right place

@asmeurer, I doubt there is such "right place". You will have to do it differently for every context (mp, fp, etc). E.g. for the mp context you, probably, should adapt defun* decorators, for the fp - _mathfun() helper and so on. I would welcome a pr, but it will definitely will be more complex than the current solution. UPD: And this will costs several tests (for dps, trap_complex, etc) per every function call.

[skirpichev]

https://peps.python.org/pep-0726/

[skirpichev]

To be concrete: if the docs in the dev tree are are still recommending mp.dps = 64 in 2024, then I suggest merging this.

Ok, I'm going to merge this in few days. Regardless on the PEP fate (python/steering-council#216) - we don't have yet a better workaround for the issue.

[skirpichev]

Thanks

[mdhaber]

Thanks, all!