These are chat archives for symengine/symengine

26th
Feb 2016
myluszczak
@myluszczak
Feb 26 2016 04:45
I've pushed two commits to Pull request 819, today. Both commits failed to pass continuous-integration/appveyor/pr. For previous commits, I've been able to see the console outputs of the failed tests by clicking on the red x and clicking on "details", but now when I mouse over the red x I just see a message "Failure: Appveyor was unable to build non-mergeable pull request." I looked through some of the other open pull requests and was unable to find another with the same problem. Is there some way I can see the console output of the test?
What I commited compiled on my computer.
Akash Trehan
@CodeMaxx
Feb 26 2016 07:50
@myluszczak Appveyor gives this error in case of possible merge conflicts. The appveyor build doesn't start in case of merge conflicts so there is no console output. You need to merge your changes with the current master(upstream) , resolve merge conflicts and then push it to github.
Isuru Fernando
@isuruf
Feb 26 2016 09:07
@bluescarni, encountered a move issue.
    integer_class _mod(0);
    for (const auto &it: prime_mul) {
        mp_pow_ui(_mod, it.first->as_mpz(), it.second);
        moduli.push_back(integer(std::move(_mod)));
        ret_val = _nthroot_mod_prime_power(rem, a->as_mpz(), n->as_mpz(), it.first->as_mpz(), it.second, false);
        if(not ret_val) return false;
    }
Francesco Biscani
@bluescarni
Feb 26 2016 12:12
@isuruf can you walk me through what is going on?
is the problem showing up at the second iteration of the loop, when _mod is in a moved-from state?
Francesco Biscani
@bluescarni
Feb 26 2016 12:17
and does it happen with Piranha's integer class or with another implementation?
Isuru Fernando
@isuruf
Feb 26 2016 12:21
With my implementation
problem is at the second iteration
Francesco Biscani
@bluescarni
Feb 26 2016 12:50
the problem is that _mod is left in a state that is only destructible and assignable then?
Isuru Fernando
@isuruf
Feb 26 2016 12:50
yes
Francesco Biscani
@bluescarni
Feb 26 2016 12:51
ok in this specific case maybe it's better to just create _mod in the loop then
since I am thinking that you need anyway some allocated space in _mod (in general)
I mean, logically at each iteration step you always need to do one allocation at least right? since you need to add an element to the vector, so you need a deep copy of that element
Isuru Fernando
@isuruf
Feb 26 2016 12:52
Yes, in this case it is. I just wanted to point out that in almost all operations, we need to check that the pointer is valid
Francesco Biscani
@bluescarni
Feb 26 2016 12:53
right I see
you could also write it without the move operation
it might end up being faster actually
because then _mp_pow_ui() needs fewer reallocations internally
if you keep _mod alive throughout loop iterations
the general pattern of GMP functions is that they allocate memory on-demand
so if your _mod already has enough limbs allocated it will be faster
if _mod is re-inited as zero each time, it will have 1 limb allocated the moment it enters _mp_pow_ui()
so reallocations might occur internally in _mp_pow_ui()
Isuru Fernando
@isuruf
Feb 26 2016 12:57
Got it. thanks
Francesco Biscani
@bluescarni
Feb 26 2016 12:57
on the other hand, reusing _mod through the iterations might eventually mean that when you copy it to add it to the vector it has many more limbs allocated than necessary... GMP functions never shrink the size of a number
so it will depend a bit on usage patterns :)
no problem
Isuru Fernando
@isuruf
Feb 26 2016 12:58
Ah, so that means even if two mpz_t's are equal their internal state might be different
Francesco Biscani
@bluescarni
Feb 26 2016 12:59
yes exactly, the number of allocated limbs can in principle be much greater than the number of limbs used in the representation of the number
there's a mpz_realloc2() function that can be used to shrink the number of allocated limbs
but I think it can quickly get complicated to have this sort of thing done automatically
Isuru Fernando
@isuruf
Feb 26 2016 13:42
@bluescarni,
inline void mp_divexact(piranha::integer &q, const piranha::integer &a, const piranha::integer &b) {
    std::cout << a << " / " << b << " = ";
    mpz_divexact(q._get_mpz_ptr(), a.get_mpz_view(), b.get_mpz_view());
    std::cout << q << std::endl;
}
this gives me 18 / 2 = 8704112
This is called by mp_divexact(q, q, p)
Isuru Fernando
@isuruf
Feb 26 2016 13:51
I assume this has something to do with the first two arguments being the same
Francesco Biscani
@bluescarni
Feb 26 2016 13:51
yep looks like it
actually I just did implement divexact in the master branch, let me check a second
Isuru Fernando
@isuruf
Feb 26 2016 13:52
Yes, I saw the PR
Is there a workaround for this, as there are other cases like this.
Francesco Biscani
@bluescarni
Feb 26 2016 13:54
it should work when the arguments are the same, but clearly there's a wider issue regarding the behaviour of mpz_view
not sure how to deal with this in the general case
I guess the only solution is to implement the functions you need directly in Piranha so I can deal with the special casing in there
I understand it's not optimal
or you can add the special casing yourself... these are all methods overloaded on different integral types right?
the behaviour of integer itself should always be safe with overlapping arguments (if it's not, it is a bug), the problems arise when the mpz_view class comes into play
Francesco Biscani
@bluescarni
Feb 26 2016 14:08
I think the most I can do regarding the mpz_view class is to add a big fat warning in the documentation
I am available to add the functionality you need in integer, I should be more responsive about it now that my son is a bit older :)
I am also not contrary to decoupling integer from piranha btw, but I would not take the lead on that
Isuru Fernando
@isuruf
Feb 26 2016 15:24
inline void mp_divexact(piranha::integer &q, const piranha::integer &a, const piranha::integer &b) {
    auto _q = get_mpz_t(q);
    auto _a = get_mpz_t(a);
    auto _b = get_mpz_t(b);
    mpz_divexact(_q, _a, _b);
 }
This works. So, I'm going to use it for now.
Francesco Biscani
@bluescarni
Feb 26 2016 15:26
bear in mind it will forcibly promote everything to dynamic storage though
oh wait... what is get_mpz_t() actually doing?
Isuru Fernando
@isuruf
Feb 26 2016 15:34
inline mpz_ptr get_mpz_t(piranha::integer &i) {
    return i._get_mpz_ptr();
}

inline auto get_mpz_t(const piranha::integer &i) -> decltype(i.get_mpz_view())  {
    return i.get_mpz_view();
}
Francesco Biscani
@bluescarni
Feb 26 2016 15:37
ah right ok... so get_mpz_t(q) promotes and get_mpz_t(a) get a view which just redirects to the internal mpz_t
Isuru Fernando
@isuruf
Feb 26 2016 15:58
Yes
Once you merge that PR, I will use the piranha method, but until then this is enough
Francesco Biscani
@bluescarni
Feb 26 2016 16:07
It should be merged already if I am not mistaken
Isuru Fernando
@isuruf
Feb 26 2016 16:11
great
Abhinav Agarwal
@abhinavagarwal07
Feb 26 2016 16:13
    void bvisit(const ACsch &x) {
        apply(result_, *(x.get_arg()));
        mpc_ui_div(result_, 1, result_, rnd_);
        mpc_asinh(result_, result_, rnd_);
    }
shouldn't order of mpc_ui_div and mpc_asinh be reversed ?
Isuru Fernando
@isuruf
Feb 26 2016 16:15
Nope. why?
@bluescarni, thanks
Francesco Biscani
@bluescarni
Feb 26 2016 16:22
np! let me know if you find anything troublesome or anything you need implemented in integer
Abhinav Agarwal
@abhinavagarwal07
Feb 26 2016 16:27
sorry my bad.
Isuru Fernando
@isuruf
Feb 26 2016 16:28
@abhinavagarwal07, no problem
Isuru Fernando
@isuruf
Feb 26 2016 16:35
@bluescarni, is there a gcd function?
something better than gcd_euclidean?
Found it
Francesco Biscani
@bluescarni
Feb 26 2016 16:45
there's an mpz_gcd wrapper yeah
http://bluescarni.github.io/piranha/doxygen/classpiranha_1_1mp__integer.html#ab16d4a27b888cfe0531d747ab646003e maybe the interface is not the one you want? would you prefer the ternary form?
Isuru Fernando
@isuruf
Feb 26 2016 16:46
this is fine
Francesco Biscani
@bluescarni
Feb 26 2016 16:47
one thing that's maybe not clear from the doc is that it's unspecified if the value returned is positive or negative... it will always be positive for positive inputs, but if at least one input is negative I think the result could be negative as well
I am working with these methods these days for the implementation of rational functions, so I am interested in getting out a decent API
Isuru Fernando
@isuruf
Feb 26 2016 16:51
One thing missing would be an equivalent of mpz_tdiv_qr
Francesco Biscani
@bluescarni
Feb 26 2016 16:53
so truncated division with quotient and remainder?
Isuru Fernando
@isuruf
Feb 26 2016 16:53
yes
there are separate functions, but not both at the same time
Francesco Biscani
@bluescarni
Feb 26 2016 16:54
ok I can do that... I'll sneak in another version of GCD in ternary form as well
Iris Lui
@irislq
Feb 26 2016 16:56
@isuruf I asked on the mailing list, but for the UnivariatePolynomial class, what would eval_bit and mul_uni_poly output since we are dealing with Expression coefficients?
Isuru Fernando
@isuruf
Feb 26 2016 16:56
eval_bit is not needed. It is a helper function for the int polynomial
mul_uni_poly should give the multiplication both polynomials
myluszczak
@myluszczak
Feb 26 2016 19:11
@CodeMaxx Thanks.
Charles Chen
@chenchfort
Feb 26 2016 23:34
Would we have to change UnivariatePolynomial in the test files to UnivariateIntPolynomial as well?