Support <,>,==,!=,<=,>=,int,bool,not operations on Constant
#72
Conversation
arcondello
force-pushed
the
array-operations
branch
from
1a2d19c
to
58d45aa
Compare
| if not self._is_scalar(): | ||
| raise ValueError("the truth value of a constant with more than one element is ambiguous") | ||
|
|
||
| return <bool>deref(self.ptr.buff()) |
There was a problem hiding this comment.
Does the buff() method point at the first (only in this case) location in memory? What is the difference between what's returned by self.ptr (I assume this is a pythonic pointer object and not actually a memory address pointer?) and self.ptr.buff()?
There was a problem hiding this comment.
self.ptr is a C++ pointer to a ConstantNode. That ConstantNode has a method buff() that returns a pointer to the beginning of the values. So this line is equivalent to static_cast<bool>(*(ptr->buff())).
| # https://stackoverflow.com/q/1521607 for the integer test | ||
| cdef double dummy | ||
| return modf(deref(self.ptr.buff()), &dummy) == <double>0.0 |
| # We could also check if rhs is another Constant and handle that differently, | ||
| # but that might lead to confusing behavior so we treat other Constants the | ||
| # same as any other symbol. |
There was a problem hiding this comment.
In what was should/could it be handled differently (unless not deferring to NumPy at all)? NumPy would just use rhs as if a NumPy array either way since it's being deferred there.
There was a problem hiding this comment.
I don't understand the question
There was a problem hiding this comment.
We could also check if rhs is another Constant and handle that differently
Just responding to this comment. Curious if there's anything that should or could be handled differently.
There was a problem hiding this comment.
Oh. The alternative would be, when both sides of the operation are Constants, to have the result be another Constant or even to "demote" to a NumPy array.
| else: | ||
| return NotImplemented # this should never happen, but just in case |
There was a problem hiding this comment.
| else: | |
| return NotImplemented # this should never happen, but just in case | |
| return NotImplemented # this should never happen, but just in case |
There was a problem hiding this comment.
I think the nesting makes it clearer that it's because those are the only options.
| operator.ne | ||
| ] | ||
|
|
||
| for op in operators: |
There was a problem hiding this comment.
(minor) It would've been nice to have this parameterized and test each operator separately.
There was a problem hiding this comment.
Isn't that what subTest does? Without needing an additional dependency.
There was a problem hiding this comment.
Oh, true. I somehow missed that line, and running the test using pytest only shows this as a single test.
There was a problem hiding this comment.
pytest-subtests required to make pytest work with subTest. Good to know!
This allows us to treat
Constantas a number in many places. And is a step towardsArraySymbol.size() -> Constant, see #50 (comment)