Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add unary operations to NumericType #5946

Merged
merged 7 commits into from Mar 30, 2023
Merged

✨ Add unary operations to NumericType #5946

merged 7 commits into from Mar 30, 2023

Conversation

Mahhheshh
Copy link
Contributor

@Mahhheshh Mahhheshh commented Mar 28, 2023

Fixes #5911

The changes made to the NumericType class include adding two new methods, __pos__ and __neg__, to support unary operations. __pos__ overloads the unary plus operator + and returns the value of the object as-is, while __neg__ overloads the unary minus operator - and returns a new instance of the same class with the negation of the value of the original object.

@ltalirz
Copy link
Member

ltalirz commented Mar 30, 2023

@sphuber would you be able to review this?

@sphuber
Copy link
Contributor

sphuber commented Mar 30, 2023

Yes, I will take care of this when I have time

@sphuber sphuber self-requested a review March 30, 2023 09:31
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @Mahhheshh . I have left two comments in the code.

return self.value

def __neg__(self):
return self.new(-self.value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also add the abs operator which is listed together with +, - and ~ as the unary operators in the official Python documentation. We can omit the ~ invert operator since that doesn't make sense for floats and I doubt anyone would want to use it for Int.

Suggested change
return self.new(-self.value)
return self.new(-self.value)

Comment on lines 219 to 241
def test_unary():
"""Test Unary operations for orm.Int and orm.Float"""

# Int
int_a = Int(5)
int_b = Int(-5)

# Float
float_a = Float(10.0)
float_b = Float(-10.0)

# Test __pos__
assert +int_a == int_a # True +5 > 5 == 5
assert +int_b == int_b # True +(-5) > -5 == -5
assert +float_a == float_a # True +10.0 > 10 == 10
assert +float_b == float_b # True +(-10.0) > -10.0 == 10.0

# Test __neg__
assert -int_a != int_a # True -(5) != 5
assert -int_b != int_b # True -(-5) != -5
assert -float_a != float_a # True (-10.0) != 10.0
assert -float_b != float_b # True -(-10.0) > 10.0 != -10.0
assert -int_a == -int_a # True -5 == -5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split this test in two (three when you implement abs as well) and use parametrization to do both numeric types

Suggested change
def test_unary():
"""Test Unary operations for orm.Int and orm.Float"""
# Int
int_a = Int(5)
int_b = Int(-5)
# Float
float_a = Float(10.0)
float_b = Float(-10.0)
# Test __pos__
assert +int_a == int_a # True +5 > 5 == 5
assert +int_b == int_b # True +(-5) > -5 == -5
assert +float_a == float_a # True +10.0 > 10 == 10
assert +float_b == float_b # True +(-10.0) > -10.0 == 10.0
# Test __neg__
assert -int_a != int_a # True -(5) != 5
assert -int_b != int_b # True -(-5) != -5
assert -float_a != float_a # True (-10.0) != 10.0
assert -float_b != float_b # True -(-10.0) > 10.0 != -10.0
assert -int_a == -int_a # True -5 == -5
@pytest.mark.parametrize('numeric_type', (Float, Int))
def test_unary_pos(numeric_type):
"""Test the ``__pos__`` unary operator for all ``NumericType`` subclasses."""
node_positive = numeric_type(1)
node_negative = numeric_type(-1)
assert +node_positive == node_positive
assert +node_negative == node_negative
@pytest.mark.parametrize('numeric_type', (Float, Int))
def test_unary_neg(numeric_type):
"""Test the ``__neg__`` unary operator for all ``NumericType`` subclasses."""
node_positive = numeric_type(1)
node_negative = numeric_type(-1)
assert -node_positive != node_positive
assert -node_negative != node_negative
assert -node_positive == node_negative
assert -node_negative == node_positive

Copy link
Contributor Author

@Mahhheshh Mahhheshh Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test for __abs__ is failing, I don't have idea about how it should be handled. Also I have made typo in the method name (realized now). Because of that tests were passing when ran locally.

Comment on lines 250 to 256
# Test positive number
abs_positive = node_positive.abs()
assert abs_positive == node_positive

# Test negative number
abs_negative = node_negative.abs()
assert abs_negative != node_negative
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __abs__ method is invoked by the abs built-in, so I would test it as follows:

Suggested change
# Test positive number
abs_positive = node_positive.abs()
assert abs_positive == node_positive
# Test negative number
abs_negative = node_negative.abs()
assert abs_negative != node_negative
assert abs(node_positive) == node_positive
assert abs(node_negative) != node_negative
assert abs(node_negative) == node_positive

@Mahhheshh Mahhheshh requested a review from sphuber March 30, 2023 16:47
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go. Thanks a lot for the contribution @Mahhheshh

@sphuber sphuber merged commit a298c14 into aiidateam:main Mar 30, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the support of unary operators (+ and -) for orm.Int and orm.Float
3 participants