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

Tensor Module #267

Merged
merged 6 commits into from Aug 8, 2014

Conversation

Projects
None yet
2 participants
@sushant-hiray
Copy link
Contributor

sushant-hiray commented Aug 3, 2014

This will comprise:

  • KroneckerDelta
  • LeviCivita

KroneckerDelta makes uses of the Assumptions Module to define some more properties such as is_above_fermi is_below_fermi. These will currenly not be implemented.

LeviCivita comprises of variable number of arguments. There is an answer for this SO Question which deals with the variable arguments case. This will be implemented after further discussion.


RCP<const Basic> kronecker_delta(const RCP<const Basic> &i, const RCP<const Basic> &j)
{
RCP<const Basic> diff = expand(sub(i, j));

This comment has been minimized.

@sushant-hiray

sushant-hiray Aug 3, 2014

Author Contributor

Calling expand here could be quite time consuming. However even basic examples such as:

>>> KroneckerDelta(i, i + 1)
        0

won't be simplified. This is because in the dict it is inserted as {i, 1} and {1+1, 1} , as a result it remains Add and not Number as expected to simplify it further.

This comment has been minimized.

@certik

certik Aug 5, 2014

Contributor

So it remains as i-(i+1)?

This comment has been minimized.

@certik

certik Aug 5, 2014

Contributor

I was thinking of keeping 2*(x+y) as is, but expanding -(x+y) automatically, precisely for this reason. I think that's what Mathematica does.

This comment has been minimized.

@sushant-hiray

sushant-hiray Aug 5, 2014

Author Contributor

Yes it remains as i-(i+1).

This comment has been minimized.

@certik

certik Aug 7, 2014

Contributor

Ok. Add a comment that the reason to call expand here is to simplify i-(i+1) to -1.

@sushant-hiray

This comment has been minimized.

Copy link
Contributor Author

sushant-hiray commented Aug 4, 2014

I tried using variadic function to implement LeviCivita function, however it seems that va_arg can only work on certain parameter types.
It raises the following error:

error: cannot receive objects of non-trivially-copyable type ‘class CSymPy::RCP<const CSymPy::Basic>’ through ‘...’; 

So for the time being, I'm using vec_basic as the parameter.

@certik

This comment has been minimized.

Copy link
Contributor

certik commented Aug 4, 2014

Don't use va_arg, it's not fully type safe. In C++, use variadic templates
if you need variable number of arguments. But better is to avoid such
things if possible, perhaps using std::vector or other classes as
arguments.

Sent from my mobile phone.
On Aug 4, 2014 8:41 AM, "Sushant Hiray" notifications@github.com wrote:

I tried using variadic function to implement LeviCivita function, however
it seems that va_arg
http://en.cppreference.com/w/cpp/utility/variadic/va_arg can only work
on certain parameter types.
It raises the following error:

error: cannot receive objects of non-trivially-copyable type ‘class CSymPy::RCP’ through ‘...’;


Reply to this email directly or view it on GitHub
#267 (comment).

Added API for LeviCivita class and its function definitions
Stuff to be done:
* complete the levicivita function
* update is_canonical accordingly
@certik

This comment has been minimized.

Copy link
Contributor

certik commented Aug 5, 2014

Looks good so far.

Updated function of `levi_civita` function. Added tests.
* Also `is_canonical` has been updated accordingly
@sushant-hiray

This comment has been minimized.

Copy link
Contributor Author

sushant-hiray commented Aug 5, 2014

@certik I've updated the levi_civita function to replicate the implementation of SymPy. Also added some basic tests. Please have a look.

sushant-hiray added some commits Aug 6, 2014

Merge branch 'master' into tensor
Conflicts:
	src/functions.cpp
	src/tests/basic/test_functions.cpp
};

//! Canonicalize LeviCivita:
RCP<const Basic> levi_civita(vec_basic arg);

This comment has been minimized.

@certik

certik Aug 7, 2014

Contributor

I think the function signature should be:

RCP<const Basic> levi_civita(const vec_basic &arg);
return s;
}

RCP<const Basic> eval_levicivita(vec_basic arg, int len)

This comment has been minimized.

@certik

certik Aug 7, 2014

Contributor

Use const vec_basic &arg.

@certik

This comment has been minimized.

Copy link
Contributor

certik commented Aug 7, 2014

Apart from my 3 comments above, it is +1 to merge.

Updated function signature for Tensor Module.
* updated vec_basic arg to const vec_basic& arg
* updated vec_basic&& arg to const vec_basic&& arg
@sushant-hiray

This comment has been minimized.

Copy link
Contributor Author

sushant-hiray commented Aug 7, 2014

I've made the required changes.

@certik

This comment has been minimized.

Copy link
Contributor

certik commented Aug 7, 2014

+1 to merge if tests pass.

sushant-hiray added a commit that referenced this pull request Aug 8, 2014

@sushant-hiray sushant-hiray merged commit f0ca864 into symengine:master Aug 8, 2014

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default All builds succeeded on Shippable
Details

@sushant-hiray sushant-hiray deleted the sushant-hiray:tensor branch Aug 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment