2
\$\begingroup\$

So I'm trying to understand the concept of descriptors and gathered some code together and made an exampel out of different sources. Like the offical documentation and StackOverflow answers. I'd hope someone can me tell some tricks about this concept or how to make this code better in general. Thanks in advanced.

PS: If someon could tell me why celsius needs to be 9_007_199_254_740_991 + 1 higher instead of just * += 1 * would be nice to know.

from abc import ABC

class Validator(ABC):
    
    def __set_name__(self,owner,name):
        self.private_name = '_' + name

    def __get__(self,obj,objtype=None):
        return getattr(obj,self.private_name)
    def __set__(self,obj,value):
        self.validate(obj,value)
        rval = round(value,3)
        setattr(obj,self.private_name,rval)
        return obj._notify(self.private_name.replace('_',''))

    def validate(self,obj,value):
        if not isinstance(value, (int, float)):
            raise TypeError(f'Expected {value!r} to be an int or float')
        if self.minvalue is not None and value < self.minvalue:
            raise ValueError(
                f'Expected {value!r} to be at least {self.minvalue!r}'
            )
        if self.maxvalue is not None and value > self.maxvalue:
            raise ValueError(
                f'Expected {value!r} to be no more than {self.maxvalue!r}'
            )

class Celsius(Validator):
    minvalue = -273.150 #Absolute zero
    maxvalue = 1.42*pow(10,32) #Planck temperature

class Farenheit(Validator):
    minvalue = -459.670 #Absolute zero
    maxvalue = 2.55*pow(10,32) #Planck temperature

class Kelvin(Validator):
    minvalue = 0 #Absolute zero
    maxvalue = 1.417*pow(10,32) #Planck temperature

class Temperature( object ):
    celsius= Celsius()
    farenheit= Farenheit()
    kelvin = Kelvin()
    def __init__(self,typ='celsius',val=0.0):
        setattr(self,typ,val)

    def _notify(self,changed_by):
        lst = ['celsius','farenheit','kelvin']
        lst.remove(changed_by)
        val = getattr(self,changed_by)
        for i in lst:
            con = getattr(Temperature,f'convert_{changed_by}_to_{i}')(val)
            rcon= round(con,3)
            setattr(self,'_'+i,rcon)
    
    @staticmethod
    def convert_kelvin_to_celsius(kelvin) -> celsius:
        return kelvin-273.15
    @staticmethod
    def convert_celsius_to_kelvin(celsius) -> kelvin:
        return celsius+273.15
    @staticmethod
    def convert_celsius_to_farenheit(celsius) -> farenheit:
        return celsius*1.8+32
    @staticmethod
    def convert_kelvin_to_farenheit(kelvin) -> farenheit:
        return kelvin*1.8-459.67
    @staticmethod
    def convert_farenheit_to_kelvin(farenheit) -> kelvin:
        return (farenheit+459.67)*1.8
    @staticmethod
    def convert_farenheit_to_celsius(farenheit) -> celsius:
        return (farenheit-32)*1.8

if __name__ == '__main__':
    t = Temperature(typ='celsius',val=0.0)
    print(t.kelvin,t.farenheit)
    t.celsius = -273.15
    print(t.kelvin,t.farenheit)
    t.celsius = 1.42*pow(10,32)
    print(t.kelvin,t.farenheit)
    t.celsius += 9_007_199_254_740_991 #+ 1

Output

273.15 32.0
0.0 -459.67
1.42e+32 2.556e+32
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

I'm not in love with the descriptor pattern in general: it lends itself poorly to immutable data. Assuming for now that we're sticking with descriptors,

  • Your validation is redundant since you only have criteria that apply to Kelvin, but then repeat those criteria for every unit. Just express this once.
  • You don't need abstract classes and you don't need inheritance. You can instantiate three descriptors, one for each unit.
  • For better or worse, the typical Python approach is so-called duck typing: if it walks like a duck, and quacks like a duck, it might as well be a duck. In this approach, you would not isinstance, and would apply your conversions and comparisons in the hope that whatever value goes in has the appropriate operators implemented. This will also help to accommodate exotic numeric classes.
  • Add PEP484 type hints.
  • Do not express a conversion for every single unit pair. Only express conversions to and from the "native" unit of Kelvin. Since all conversions are linear, express this via offset and coefficient.
  • Add unit tests.
  • Stop writing pow(10, for literals; use e notation. It doesn't matter if you're only doing this once: exponential notation is more concise, easier to read and easier to optimise (if optimisation ends up being needed). It's also a better match to the output format used by basically all computers, running Python programs or otherwise, for very small or large numbers.
  • Spelling: farenheit -> fahrenheit
  • Don't inherit from object; we aren't in Python 2. You claim to do this because it's more explicit, but I'm going to claim that that guideline doesn't apply here: a bare class declaration will be a mystery to no one.

Suggested

import math
from numbers import Real
from typing import Type


class TemperatureDescriptor:
    def __init__(self, coeff: Real, offset: Real) -> None:
        self.coeff, self.offset = coeff, offset

    def __get__(self, obj: 'Temperature', objtype: Type['Temperature']) -> float:
        return obj._kelvin*self.coeff - self.offset

    def __set__(self, obj: 'Temperature', value: Real) -> None:
        k = (value + self.offset)/self.coeff
        if k < 0:
            raise ValueError('Temperature is less than absolute zero')
        if k > 1.42e32:
            raise ValueError('Temperature is greater than the Planck limit')
        obj._kelvin = k


class Temperature:
    kelvin = TemperatureDescriptor(1, 0)
    celsius = TemperatureDescriptor(1, 273.15)
    fahrenheit = TemperatureDescriptor(1.8, 273.15*1.8 - 32)


def test() -> None:
    def isclose(expected: Real, actual: Real) -> None:
        assert math.isclose(expected, actual, rel_tol=0, abs_tol=1e-12)

    t = Temperature()
    t.celsius = 30
    isclose(30, t.celsius)
    isclose(86, t.fahrenheit)
    isclose(303.15, t.kelvin)

    t.celsius = 0
    isclose(0, t.celsius)
    isclose(32, t.fahrenheit)
    isclose(273.15, t.kelvin)

    t.celsius = 100
    isclose(100, t.celsius)
    isclose(212, t.fahrenheit)
    isclose(373.15, t.kelvin)

    t.fahrenheit = 0
    isclose(0, t.fahrenheit)
    isclose(-17.777777777778, t.celsius)
    isclose(255.372222222222, t.kelvin)

    t.kelvin = 0
    isclose(0, t.kelvin)
    isclose(-273.15, t.celsius)
    isclose(-459.67, t.fahrenheit)

    try:
        t.kelvin = -1
        raise AssertionError()
    except ValueError:
        pass

    try:
        t.kelvin = 2e32
        raise AssertionError()
    except ValueError:
        pass


if __name__ == '__main__':
    test()
\$\endgroup\$
4
  • \$\begingroup\$ Sorry for my late reply, I wasnt home for quite a while and do not had access to my account here. I have some questions why I should do what you recommand. What is the benefit of use e notation here, because I juste calculate it once? In addition, I started to learn descriptors because of another answer from you, I want to do my own dataclass and validate the descripted datatypes. Espacially for userinput. Thanks for reviewing my code. \$\endgroup\$ Feb 6 at 8:25
  • \$\begingroup\$ I forgot to mention my other questions. I was reading the documentation about math.isclose and there it says rel_tol must be greater than zero I see it works but dont know if it is the right approach. I constantly inherite from object because of the Zen of python and the explicit over implicit rule that comes within. \$\endgroup\$ Feb 6 at 8:41
  • \$\begingroup\$ I actually never noticed the rel_tol minimum. It's weird that they say that, since passing 0 does exactly what we would expect and is a reasonable way to ask for absolute comparison only. Further, the runtime doesn't actually enforce that limit. \$\endgroup\$
    – Reinderien
    Feb 6 at 14:06
  • 1
    \$\begingroup\$ As to your other questions, I've expanded rationale in the answer. \$\endgroup\$
    – Reinderien
    Feb 6 at 14:07
1
\$\begingroup\$

I think storing copies of the state on each of the different unit types (i.e. Celsius, Fahrenheit) and then updating them via notify makes the code a lot harder to reason over, and in this case doesn't give you much of the benefit you'd normally get from using callbacks. Temperature might not be coupled to Celsius etc, but they are very much coupled to it.

Accepting that, why not have the Celsius / Fahrenheit objects ask Temperature for its value, and have that value in a consistent format?

Additionally, it'd be quite nice to make this all a bit more dynamic, if we extract out the scales we're using into an enum, and then make some conversion methods that use that enum we can make the code a bit more readable, and avoid having to create additional classes for each new scale.


class TemperatureScales(Enum):
    KELVIN = (0, 1.417*pow(10,32))
    CELSIUS = (-273.150, 1.42*pow(10,32))
    FARENHEIT = (-459.670, 2.55*pow(10,32))
    
    def __init__(self, minvalue, maxvalue):
        self.minvalue = minvalue
        self.maxvalue = maxvalue
    
    def validate(self, value):
        if not isinstance(value, (int, float)):
            raise TypeError(f'Expected {value!r} to be an int or float')
        if self.minvalue is not None and value < self.minvalue:
            raise ValueError(
                f'Expected {value!r} to be at least {self.minvalue!r}'
            )
        if self.maxvalue is not None and value > self.maxvalue:
            raise ValueError(
                f'Expected {value!r} to be no more than {self.maxvalue!r}'
            )
        return value


def _to_kelvin(value: float, units: TemperatureScales):
    if units is TemperatureScales.KELVIN:
        return value
    if units is TemperatureScales.CELSIUS:
        return value + 273.15
    if units is TemperatureScales.FARENHEIT:
        return (value+459.67)*1.8
    raise NotImplementedError("Invalid temperature units")


def _from_kelvin(value: float, units: TemperatureScales):
    if units is TemperatureScales.KELVIN:
        return value
    if units is TemperatureScales.CELSIUS:
        return value - 273.15
    if units is TemperatureScales.FARENHEIT:
        return value*1.8-459.67
    raise NotImplementedError("Invalid temperature units")

Now we can create a descriptor which represents a particular scale, which references a '_value' attribute in its owner, we'll make this class private, as it will only be used in the context of the owner class:

class _QualifiedTemperature:
    def __init__(self, units=TemperatureScales.KELVIN):
        self._units = units
    
    def __set__(self, obj, value):
        obj._value = _to_kelvin(self._units.validate(value),
                                 units=self._units)
        pass
    
    def __get__(self, obj, objtype=None):
        return _from_kelvin(obj._value, units=self._units)

Finally we can create our owner class, we can do this dynamically in case we add any extra temperature scales later on:

def _cant_set(self, obj, value):
    raise TypeError("Can't set temperature directly, must set through a specific scale (i.e.) temp.celsius = ")

_temperature_dict = {scale.name.lower(): _QualifiedTemperature(units=scale) 
                    for scale in TemperatureScales}
_temperature_dict['__set__'] = _cant_set

Temperature = type('Temperature',
                   tuple(),
                  _temperature_dict)

We can now use this in a similar way to what you've done before, only now all setting of values is done through the units.

class SomeTemps:
    temp_a = Temperature()
    temp_b = Temperature()
    def __init__(self):
        self.temp_a.farenheit = 300
        self.temp_b.celsius = 300

>>> example = SomeTemps()
>>> example.temp_a.celsius
1094.2560000000003
>>> example.temp_a.kelvin = 10
>>> example.temp_a.celsius
-263.15
>>> example.temp_b.kelvin = -1
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-37-5502f798a99e> in <module>
----> 1 example.temp_b.kelvin = -1

<ipython-input-34-12b014367a6a> in __set__(self, obj, value)
     50 
     51     def __set__(self, obj, value):
---> 52         obj._value = _to_kelvin(self._units.validate(value),
     53                                  units=self._units)
     54         pass

<ipython-input-34-12b014367a6a> in validate(self, value)
     15             raise TypeError(f'Expected {value!r} to be an int or float')
     16         if self.minvalue is not None and value < self.minvalue:
---> 17             raise ValueError(
     18                 f'Expected {value!r} to be at least {self.minvalue!r}'
     19             )

ValueError: Expected -1 to be at least 0
\$\endgroup\$
2
  • \$\begingroup\$ I agree with your assessment of the notify mechanism. For everything else - without getting into a lot of detail - I find it, unfortunately, overbuilt and not dry enough. \$\endgroup\$
    – Reinderien
    Jan 24 at 14:54
  • \$\begingroup\$ Sorry for my late reply, I wasnt home for quite a while and do not had access to my account here. The conclusion out of your answer that I made was, I was pretty much doing just a MinMaxNumber and a seperated class for each is overpowered and redundant. Also I agree with you that the conversation needs to be simplyfied and can be done like you did. Thanks for reviewing my code. \$\endgroup\$ Feb 6 at 8:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.