These are chat archives for symengine/symengine

21st
May 2016
myluszczak
@myluszczak
May 21 2016 04:50
@shivamvats @isuruf @certik PRs #925 and #913 have been updated to remove the expansion of coefficients during the creation of the polynomials; this is a minor change and these PRs still pass all tests and can still be automatically merged. PR #924 remains unchanged. I have also submitted a new PR (#947) which in addition to the contents of PR #913 contains the MultivariateSeries class. This PR also passes all tests and can be merged automatically. If someone could review and merge these PRs, that would be great.
Isuru Fernando
@isuruf
May 21 2016 04:51
@myluszczak, why do you have the add wiki and rm wiki commits at the beginning of each PR?
Charles Chen
@chenchfort
May 21 2016 05:21
@isuruf They branched off a branch that contains those experimental commits that I made a long time ago. Let me see if I can rebase out those.
Siddharth
@bollu
May 21 2016 05:39
@isuruf Could you take a look at the PR against SymEngine.hs please?
Isuru Fernando
@isuruf
May 21 2016 05:40
@bollu, looking now
Siddharth
@bollu
May 21 2016 05:40
thanks
Isuru Fernando
@isuruf
May 21 2016 06:02
@myluszczak, are you here?
myluszczak
@myluszczak
May 21 2016 06:44
@isuruf I am here
Isuru Fernando
@isuruf
May 21 2016 06:46
I left a few comments in the PR. One question is whether you want to special case for UnivariatePolynomial
myluszczak
@myluszczak
May 21 2016 06:46
In terms of addition, multiplication, etc?
Isuru Fernando
@isuruf
May 21 2016 06:47
yes
Also is degress_ used?
myluszczak
@myluszczak
May 21 2016 06:49
On reflection, the same capability could be provided for with implicit conversion, and it would make things neater.
degrees_ would be used in an algorithm in which it is necessary to know the degree of each variable. It'd be useful to have when doing, for example, Kronecker substitution, which I expect to be doing as a step in any optimization. I don't think It's used for anything in this PR, though.
Siddharth
@bollu
May 21 2016 07:21
@isuruf - If I want to start implementing a simplification layer, where do I start from? My plan is to see how SymPy does it and then implement it in SymEngine
does that sound alright?
I'd like some pointers
Isuru Fernando
@isuruf
May 21 2016 07:22
What kinds of simplification?
Siddharth
@bollu
May 21 2016 07:22
e^(i 2pi) was what I had in mind first
reduce complex exponentiation into real and imaginary parts that is
Isuru Fernando
@isuruf
May 21 2016 07:24
See as_real_imag in SymPy and you can implement it in SymEngine
Siddharth
@bollu
May 21 2016 07:24
cool, will do
Srajan Garg
@srajangarg
May 21 2016 12:07
@isuruf I was trying to make a template for the dictionary wrapper. I have a the base class as,
template <typename Key, typename Value, typename Wrapper>
class ODictWrapper
here's the += method for this class
Wrapper &operator+=(const Wrapper &other)
{
    for (auto &iter : other.dict_) {
      // addition here
    }
    return *this;
}
and the derived class as follow
class UnivariateExprPolynomial : public ODictWrapper<int, Expression, UnivariateExprPolynomial>
Isuru Fernando
@isuruf
May 21 2016 12:09
That's good. Why do you need Key?
Srajan Garg
@srajangarg
May 21 2016 12:10
The problem here is in the function, *this is of type ODictWrapper<int, Expression, UnivariateExprPolynomial>, while the expected return is of type UnivariateExprPolynomial
What can be done?
Key because I wanted to keep it as general as possible (also UnivariateInt uses uint while UnivariatePoly uses int)
Isuru Fernando
@isuruf
May 21 2016 12:11
Okay. We should try to make UnivariateInt use int as well. But that can be done later
Srajan Garg
@srajangarg
May 21 2016 12:12
I don't see any issue with the general Key approach as of yet
Any solution to the problem with the += operator?
This is the exact error, I don't know what the best way to tackle it is
error: invalid initialization of reference of type ‘SymEngine::UnivariateExprPolynomial&’ from expression of type ‘SymEngine::ODictWrapper<int, SymEngine::Expression, SymEngine::UnivariateExprPolynomial>return *this;
I can either construct a UnivariateExprPolynomial from the ODictWrapper?
Srajan Garg
@srajangarg
May 21 2016 12:17
Something like return Wrapper(*this)
Isuru Fernando
@isuruf
May 21 2016 12:20
There are 2 options, but I'm not sure which one is best.
  1. return static_cast<Wrapper &>(*this); is one way.
  2. Changing the return type.
Srajan Garg
@srajangarg
May 21 2016 12:23
Changing the return type won't help right?
Isuru Fernando
@isuruf
May 21 2016 12:24
I think not
Not sure though
Srajan Garg
@srajangarg
May 21 2016 12:25
We want the return type to be the same as the two operands right
Isuru Fernando
@isuruf
May 21 2016 12:25
Yes. So static_cast is the only option I guess
Srajan Garg
@srajangarg
May 21 2016 12:26
Okay, i'll put a TODO comment, if we want to discuss on this later
Isuru Fernando
@isuruf
May 21 2016 12:27
@myluszczak, see StoicBronco/symengine#1
@srajangarg, can you review #948?
Srajan Garg
@srajangarg
May 21 2016 12:29
One more way would be to put the definition in operator+ (a, b). Do the addition in that function, by constructing a new dictionary there itself, and returning that. Then, we use + in the implementation of += (Basically just the opposite of the way it is right now) Hence, no static_cast will be required.
Yes, will have a look
friend Wrapper operator+(const Wrapper &a, const Wrapper &b)
{
    Wrapper c;
    // update c's dict
    return c;
}
and
Wrapper &operator+=(const Wrapper &other)
{
    return (*this + other);
}
Isuru Fernando
@isuruf
May 21 2016 12:36
Then, += is not inplace anymore
Srajan Garg
@srajangarg
May 21 2016 12:36
Yes, must it be?
Isuru Fernando
@isuruf
May 21 2016 12:37
Yes, then it will be undefined behaviour
Use static_cast
Srajan Garg
@srajangarg
May 21 2016 12:37
Okay. But why must += be always inplace?
Anywhere I can read about this?
Isuru Fernando
@isuruf
May 21 2016 12:41
+= operator is an assignment operator. http://en.cppreference.com/w/cpp/language/operator_assignment
Srajan Garg
@srajangarg
May 21 2016 14:33
@isuruf Do we need __hash()__ for our dictionary wrappers? (probably because they don't inherit from Basic)
Isuru Fernando
@isuruf
May 21 2016 14:34
I don't think so
I think UnivariateSeries needs it
Srajan Garg
@srajangarg
May 21 2016 14:35
Cool