Make use of ABC abstract class in unsequa module

[luseverin]

Changes proposed in this PR:
-Make use of ABC abstract class in the unsequa module

I updated the unsequa module so that it uses the abstract (ABC) class concept. The idea is to be able to readily adapt the unsequa module to other use cases, using the related abstract class. In my case I wanted to have unsequa work with the network module from petals. I splitted this task in two where 1) the unsequa module is made abstract (this PR); 2) the adaptation of the unsequa module to the network module use case is implemented (in branch feature/unsequa_cascade_abstract, work in progress).

As @emanuel-schmid and @chahank nicely designed the unsequa module so that it is already quite abstract in the first place, this did not require too much work. However, I was not sure of whether the __init__ of the UncOutput class should be made abstract or not, as making it abstract is preventing __init__ to be used by subclasses using super()..

PR Author Checklist

• Read the Contribution Guide
• Correct target branch selected (if unsure, select develop)
• Descriptive pull request title added
• Source branch up-to-date with target branch
• Documentation updated
• Tests updated
• Tests passing
• No new linter issues
• Changelog updated

PR Reviewer Checklist

• Read the Contribution Guide
• CLIMADA Reviewer Checklist passed
• Tests passing
• No new linter issues

[peanutfun]

What does this PR achieve that was not possible before? The new abstract methods do not serve any purpose if they do not define an interface (through parameters). But defining an interface also means that derived methods must use the same parameters. The current structure of the unsequa module already violates this, and Jenkins complains, rightfully so. See https://pylint.pycqa.org/en/latest/user_guide/messages/warning/arguments-differ.html:

argument-differ denotes an issue with the Liskov Substitution Principle.

So, if you want derived classes to change the interface, you don't need an interface in the first place. On the other hand, we could make an effort here to define a proper interface for the unsequa base classes that conforms to all derived use cases.

[luseverin]

@peanutfun Thanks for the feedback! I think the idea here was not to add a functionality, but to make use of abstract classes as it considers as "best practice"? But yes in theory you can implement other subclasses in the unsequa module without the use of an abstract class, as it has been done before for e.g. calc_delta_climate or calc_cost_benefit. But now I understand how this might be problematic as the derived classes indeed require additional parameters.. Would it be beneficial in your opinion to make this effort to define a proper interface for the unsequa base class that will be conform with the derived classes? Not sure of how much additional work this would require..

[emanuel-schmid]

making __init__ abstract looks beyond the target to me. Is there a use case?

[peanutfun]

I agree. You usually call inits of base classes from derived classes instead of overriding them.

[emanuel-schmid]

private and abstract seems somehow contradicting. if we go for it, compute_metrics ought to be public.

[peanutfun]

I disagree. An abstract method just means that "this implementation is to be defined in a derived class". Depending on the semantics of the class, the method in question may very well be private.

[emanuel-schmid]

@peanutfun Technically speaking, you're absolutely right. In this particular case I still think compute_metrics ought to be public.

[emanuel-schmid]

I kind of agree with Lukas. Up to this point the abstraction is not justified yet (and not yet according to all rules).
However - I can see (or hunch) its potential. (;
Only, in this case, I'd rather combine the implementation of the abstraction and its use in one PR.

[chahank]

@emanuel-schmid and @peanutfun I am not sure what you want to happen here. I think adding the abstract structure would make it much simpler for future developers to add functionalities, in the same way as @luseverin is currently doing. But happy to be corrected.

[chahank]

@luseverin : small point, but it seems that some modification of the tutorial happened that might not have been intended (2,182 additions, 478 deletions)

[peanutfun]

I think adding the abstract structure would make it much simpler for future developers to add functionalities, [...]

@chahank Please refer to my earlier comment: #1054 (review). The only way to make things simpler is to define a proper interface in the base class(es). But this is neither done here nor was it done originally.

[chahank]

Thanks, but this does not answer the question: what do you want to happen here? I think the current comments do not allow @luseverin to either improve, or close this PR. Not sure what is best: put in the effort to make it into a proper abstract structure, or just close this.

[emanuel-schmid]

Sorry for not being clear. In order to continue with this PR, I would want

• a proper definition of the interface
• a use case where this interface is used / called, which justifies the abstraction