RealDouble class #439
RealDouble class #439
Conversation
|
I added automatic evaluation to some of the functions using single_dispatch. |
isuruf
force-pushed
the
real
branch
2 times, most recently
from
98cfab7
to
a6110f7
Compare
| }; | ||
| virtual Evaluate* get_eval() const { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
You should never return a raw pointer like this. In this particular case, since Evaluate is a small class with no members, you can just return Evaluate by value. In more complicated cases, you need to use a smart pointer, perhaps std::unique_ptr.
There was a problem hiding this comment.
I forgot to remove these methods. Evaluate cannot be returned by value since it is an abstract class, we could use std::unique_ptr.
There was a problem hiding this comment.
I see. The problem with both RCP and unique_ptr is that they require to call new, so it's going to be very slow compared to other options.
There was a problem hiding this comment.
using RCP slows down things considerably. We'll go with the single dispatch and look at two argument functions later.
Since this PR is getting big, shall I remove automatic evaluations and send it as a separate PR?
There was a problem hiding this comment.
Either way is fine, whatever is easier to review / get in.
As to pointers, I created #445 with more details.
|
@certik, this is now ready for review. |
| NUMBER, INTEGER, RATIONAL, COMPLEX, CONSTANT, | ||
| INTEGER, RATIONAL, COMPLEX, REAL_DOUBLE, NUMBER, | ||
| // 'NUMBER' returns the number of subclasses of Number. | ||
| // All subclasses of Number must be added before it. |
There was a problem hiding this comment.
How about renaming NUMBER to TypeID_Number_Count, to make it clear what it does?
There was a problem hiding this comment.
Another problem with this is that now TypeID_Count is off by 1, or to be precise, the NUMBER slot is not being used.
There was a problem hiding this comment.
Is this only used in is_a_Number? If so, perhaps we can find a different solution.
There was a problem hiding this comment.
Yes, it's only used by is_a_Number at the moment. I'll use REAL_DOUBLE instead.
|
Otherwise I think it looks good. But we also need to run benchmarks to make sure things didn't get slower. |
| const RealDouble &s = static_cast<const RealDouble &>(o); | ||
| return this->i == s.i; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Should this method just return false without the checks?
There was a problem hiding this comment.
I think it's fine as it is --- the only defined comparison is if the double is the same variable, perhaps pointed to from two different pointers. Which is what you are doing. I think that's fine, otherwise it would be confusing if you did:
a = 1.0
b = a
and then a == b would return False.
|
I ran |
| } else { | ||
| Mul::as_base_exp(b, outArg(exp), outArg(t)); | ||
| Mul::dict_add_term_new(outArg(coef), d, exp, t); | ||
| } |
There was a problem hiding this comment.
This change has made an unexpected speedup.
There was a problem hiding this comment.
Why do you think it is faster? Probably a faster number handling?
There was a problem hiding this comment.
There's a slight difference on what they do. When multiplying 2**x by 2 previously we got 2**(x+1), but multiplying 2**x*y by 2 we got (1/2)*2**x*y. This change makes them all behave as the latter. Therefore avoids the dictionary lookup.
|
Very cool. +1 to merge Sent from my mobile phone.
|
No description provided.