MilaNedic / moarchiving

This repository contains the description and corresponding files for the "Computing and updating Hypervolume" project
MIT License
0 stars 0 forks source link

Setting class variable when using factory method #8

Closed naceee closed 2 weeks ago

naceee commented 2 weeks ago

We have 3 classes, that are dealing with archiving points in 2, 3 and 4 dimensions: let's call them class2, class3 and class4. Each class needs to have implemented certain methods, lets say method1 and method2, that's why we have an abstract base class called AbstractClass.

class AbstractClass(ABC):
    @abstractmethod
    def method1(self):
        pass

    @abstractmethod
    def method2(self):
        pass

Additionally class3 and class4 (but not class2) share some common methods - let's say method1 in this example, so we create a parent class for them called Parent34, to avoid duplicating code.

class Parent34(AbstractClass):
    m = 42

    def __init__(self, n):
        self.m = Parent34.m
        self.n = n

    def method1(self):
        # does something
        pass

    def method2(self):
        raise NotImplementedError("method2 is implemented in subclasses")

We also want to have one class variable m set to 42 for all of the classes by defualt, but we might want to change this. So our base classes look like this.

class Class2(AbstractClass):
    m = 42

    def __init__(self):
        self.m = Class2.m
        self.n = 2

    def method1(self):
        # does the same thing as method1 in Parent34, but in a different way
        pass

    def method2(self):
        # does something
        pass

class Class3(Parent34):
    def __init__(self):
        super().__init__(n=3)

    def method2(self):
        # does the same thing as method2 in Class2, but in a different way
        pass

class Class4(Parent34):
    def __init__(self):
        super().__init__(n=4)

    def method2(self):
        # does the same thing as method2 in Class2 and Class3, but in a different way
        pass

To make the different classes easy to use for the user, we also have a factory function ClassFactory, where we return the correct class for the given n.

def ClassFactory(n):
    if n == 2:
        return Class2()
    elif n == 3:
        return Class3()
    elif n == 4:
        return Class4()
    else:
        raise ValueError("unsupported n")

Sometimes we would like to change the class variable m before creating a new instance of a class. Without using the factory method we could do something like this:

Class2.m = 43
c2 = Class2()
print(c2.m)
> 43

But we would like the user to be able to do this without specifying the class Class2. How can we do this?

nikohansen commented 2 weeks ago

Solution one: it can be a class attribute of AbstractClass?

Solution two (probably better): it can become a module-global variable instead of a class variable?

In both cases the code within the classes need to be adapted to account for the different variable name.

ttusar commented 2 weeks ago

At a call with Niko and Olaf we came to the following conclusions:

nikohansen commented 2 weeks ago

A possible aspect: the module should ideally not depend on Fraction to be installed, even if it is (decided to be) the default type for the hypervolume.

naceee commented 2 weeks ago

Do you think it is a reasonable assumption, that the different classes will always be initialized using the GetArchive function or not?

nikohansen commented 2 weeks ago

Do you think it is a reasonable assumption, that the different classes will always be initialized using the GetArchive function or not?

Probably not, because this would break backwards compatibility which is generally not desirable (at all).

nikohansen commented 2 weeks ago

It is also be bug prone to assume the user is not using a class directly and it creates an unnecessary dependency when the class is not self sustained. If such assumption is made, at the very least the class names must start with an underscore to make it clear that they are for internal use only.

naceee commented 2 weeks ago

Thanks, that makes sense and I implemented it this way - so that every class can be used on it's own as well. So I think I can close this issue now.