Open arcondello opened 3 years ago
Also .vartype
while we're at it...
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?
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).
I hear the extensibility argument. If we proceed, I would be careful to keep interfaces segregated, unless BQM will subclass QM.
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.
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.
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.
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:
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()
.[break compat]
again to remove get_X()
, or just live with aliasesFWIW, 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).
Currently,
BQM.num_variables
,BQM.num_interactions
andBQM.shape
are all properties rather than methods. I think that it would be better to make them proper methods.Pro:
num_interactions
is anO(num_variables)
operation, so having it as a property is misleadingbqm.num_interactions(v)
could return the degree ofv
DQM.num_variables()
andDQM.num_cases()
are currently implemented as methods, which is inconsistentnum_interactions
andnum_variables
are already read-only. As a method that would be even more explicitCon:
.shape
as a property is consistent with NumPyAdditional context If we don't want to break backwards compatibility through a deprecation period
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