Consider making BQM.num_variables, BQM.num_interactions and BQM.shape methods rather than properties

[arcondello]

Currently, BQM.num_variables, BQM.num_interactions and BQM.shape are all properties rather than methods. I think that it would be better to make them proper methods.

Pro:

• Determining num_interactions is an O(num_variables) operation, so having it as a property is misleading
• Potentially extendable in the future, e.g. bqm.num_interactions(v) could return the degree of v
• DQM.num_variables() and DQM.num_cases() are currently implemented as methods, which is inconsistent
• num_interactions and num_variables are already read-only. As a method that would be even more explicit

Con:

• Backwards compatibility break, though I think we can mitigate by making them inherit from numeric and callable (see below)
• Having .shape as a property is consistent with NumPy

Additional context If we don't want to break backwards compatibility through a deprecation period

from collections.abc import Callable

class CallableInt(int, Callable):
    def __call__(self):
        return self

works out of the box. Though getting it to raise a deprecation warning if it's not treated as a callable would require something like

import warnings

from collections.abc import Callable
from numbers import Integral

class CallableInt(Integral, Callable):
    def __init__(self, value):
       self.value = int(value)

    def __call__(self):
        return self.value

    def __add__(self, other):
        warnings.warn("In the future will be callable", DeprecationWarning)
       return self.value + other

    ...

[arcondello]

Also .vartype while we're at it...

[randomir]

Functionally, it makes perfect sense to me to keep all of those as properties.

num_variables (or num_interactions or vartype) is a property of BQM, regardless of implementation details, and the fact that determining the actual value involves some calculation, or it has > O(1) complexity.

OTOH, I also get the performance argument, and "property calc should be cheap".

A middle ground would be caching. Either of property/method result (in which case invalidation is a pain I assume), or keeping actual num_variables/etc attributes up to date on each operation?

[arcondello]

To hone in on a specific example that this has already come up on, let's compare BQM.vartype (property) and QM.vartype(v) (method).

If I want to write a function that takes either, I have to do something like

for v in model.variables:
    if isinstance(model, BQM):
       vt = model.vartype
    else:
       vt = model.vartype(v)

whereas, if we implemented vartype as a method in BQM

def vartype(self, v=None):
    return self._vartype

for BQM, we could have nice generic code.

I think the main reason for the proposal is that methods are more flexible in the long run, they more easily allow extensions later (like the above).

[randomir]

I hear the extensibility argument. If we proceed, I would be careful to keep interfaces segregated, unless BQM will subclass QM.

[arcondello]

Of course, though they do already share part of their interface by inheritance (QM, BQM).

In my view a set of generic interfaces across all quadratic models makes sense.

[arcondello]

I tried a few approaches, and I think

import warnings


class MyInt(int):
    def __new__(cls, method):
        obj = super().__new__(cls, method())
        obj._method = method
        return obj

    def __call__(self, *args, **kwargs):
        return self._method(*args, **kwargs)
    
    # call out a few common methods to raise a deprecation warning
    def __eq__(self, other):
        warnings.warn("Accessing num_variables as a property is deprecated", DeprecationWarning)
        return int(self) == other


class A:
    def _num_variables(self):
        # method version
        return 5

    @property
    def num_variables(self):
        return MyInt(self._num_variables)

ends up being the best.

I really wanted something like

    def __getattribute__(self, name):
        if name in numbers.Integera.__abstractmethods__:
            warnings.warn(...)
        ...

to work, but sadly it mostly gets bypassed.

Another approach would be to fully use numbers.Integral abc, but that gets very tedious and IMO error prone.

[arcondello]

Another approach, based on a suggestion by @thisac, would be something like

future.py

class NewFeature:
    pass

myclass.py

import inspect

from future import NewFeature


class A:
    @property
    def is_method(self):
        caller = inspect.currentframe().f_back
        if caller.f_globals.get("NewFeature", None) is NewFeature:
            return lambda: True

        # throw a deprecation warning here

        return False

main.py

from myclass import A

a = A()

assert a.is_method is False

from future import NewFeature

assert a.is_method() is True

This is nice because it allows individual namespaces to decide to opt into the future behavior. On the other hand, it's CPython specific and requires a pretty expensive lookup on each method call.

[randomir]

I like the from future import feature approach because it's simple-ish and explicit.

If performance hit is too high (TBD?), we can break compatibility only once, but max twice:

• add get_X() methods in addition to X properties, and raise deprecation on X's use
• [break compat] remove X properties, deprecate get_X() methods (with a long to indefinite deprecation period - these would just call new X() methods), make X() methods that mirror old get_X().
• optionally [break compat] again to remove get_X(), or just live with aliases

FWIW, the proxy route seems too messy, as there will surely be edge cases we'll miss. OTOH, automating the second approach to infer if the next op in ~dis.dis(stack_frame) is a call (return lambda) or not, seems too finicky (and reminds me of our @vartype_argument saga from the old days).