Implements equality methods for impf and impfset #1027
Conversation
| @@ -97,6 +97,20 @@ def __init__( | |||
| self.mdd = mdd if mdd is not None else np.array([]) | |||
| self.paa = paa if paa is not None else np.array([]) | |||
|
|
|||
| def __eq__(self, value: object, /) -> bool: | |||
| if isinstance(value, ImpactFunc): | |||
There was a problem hiding this comment.
I think this has the issue that derived classes won't be recognized (e.g. ImpfStormEurope).
There was a problem hiding this comment.
Nope,
See for instance: https://stackoverflow.com/a/152596/4703808
There was a problem hiding this comment.
(Which makes sense, a derived class is indeed an instance of the base class, i.e., a Square is a Rectangle, just with additional specificities :-) )
There was a problem hiding this comment.
Can you please double check and make a test to be sure?
There was a problem hiding this comment.
I don't think this is necessary. It is the default behavior.
|
I appreciate the effort, but this looks like boilerplate code to me. If users add new attributes, they will likely miss updating these methods. Wouldn't a simple |
|
What do you mean @peanutfun ? This is the impact function set class and this is about implementing the equality method. |
|
@chahank Adding |
There was a problem hiding this comment.
I still feel like we do not need to check the actual data inside the ImpactFunc to determine equality. But I am fine with the current approach that ensures lazy evaluation of the data arrays.
I added some suggestions for possible improvement.
Good job with the testing! It's so extensive that I almost feel like it's too much given the amount of "actual" code changes. But I also don't know which tests one could safely drop.
Notice that tests for ImpactFuncSet are still missing.
Co-authored-by: Lukas Riedel <[email protected]>
Co-authored-by: Lukas Riedel <[email protected]>
| @@ -97,6 +97,20 @@ def __init__( | |||
| self.mdd = mdd if mdd is not None else np.array([]) | |||
| self.paa = paa if paa is not None else np.array([]) | |||
|
|
|||
| def __eq__(self, value: object, /) -> bool: | |||
| if isinstance(value, ImpactFunc): | |||
There was a problem hiding this comment.
I don't think this is necessary. It is the default behavior.
Co-authored-by: Lukas Riedel <[email protected]>
Co-authored-by: Lukas Riedel <[email protected]>
While working on the Cost Benefit refactoring I need to test the equality between diverse climada objects,
ImpactFuncSet among other. I thought it would be worth to implement the functions as
__eq__methods.Changes proposed in this PR:
__eq__methods (==) for ImpactFunc__eq__methods (==) for ImpactFuncSetPR Author Checklist
develop)PR Reviewer Checklist