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

Create operators.py file #12696

Merged
merged 12 commits into from Jun 7, 2017

Conversation

Projects
None yet
2 participants
@szymag
Contributor

szymag commented May 31, 2017

Create classes Gradient, Curl, Divergence where expression
is not evaluated.

Functions curl, divergence, gradient now
are localized in operators.py file.

Argument coord_sys in gradient, curl, divergence, have default value None.
This parameter is obtained from vector or scalar itself in function _get_coord_sys_from_expr

Create operators.py file.
Create classes Gradient, Curl, Divergence where expresion
is not evaluated. Functions curl, divergence, gradient now
create object from proper class and evaluate expression.

@szymag szymag changed the title from Create operators.py file to [WIP]Create operators.py file May 31, 2017

Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr May 31, 2017

Contributor

Some tests for the new additions?

Contributor

Upabjojr commented May 31, 2017

Some tests for the new additions?

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr May 31, 2017

Contributor

Tests should just check that .doit() works and that expressions are not evaluated if no .doit() is called.

Contributor

Upabjojr commented May 31, 2017

Tests should just check that .doit() works and that expressions are not evaluated if no .doit() is called.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag May 31, 2017

Contributor

Tests should just check that .doit() works and that expressions are not evaluated if no .doit() is called.

OK, I will do that

Contributor

szymag commented May 31, 2017

Tests should just check that .doit() works and that expressions are not evaluated if no .doit() is called.

OK, I will do that

Apply sugestion from comments.
Add tests, change inheritance, argument
coord_sys have default value None.

@szymag szymag changed the title from [WIP]Create operators.py file to [WIP] Create operators.py file May 31, 2017

Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
Remove coord_sys parameter from constructor.
Add method _get_coord_sys_from_expr.
@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag May 31, 2017

Contributor

I applied your suggestion. I think that now it has more sense.
Now, functions gradient, curl, divergence can return value in two ways, as before and using new classes.
I think that it's worthwhile to add description about it.

Contributor

szymag commented May 31, 2017

I applied your suggestion. I think that now it has more sense.
Now, functions gradient, curl, divergence can return value in two ways, as before and using new classes.
I think that it's worthwhile to add description about it.

Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
assert Gradient(s1) == Gradient(R.x*R.y*R.z)
assert Gradient(s2) == Gradient(R.x + 3*R.y**2)
assert Gradient(s1).doit() == R.y*R.z*R.i + R.x*R.z*R.j + R.x*R.y*R.k
assert Gradient(s2).doit() == R.i + 6*R.y*R.j

This comment has been minimized.

@Upabjojr

Upabjojr May 31, 2017

Contributor

OK, that's what I meant.

@Upabjojr

Upabjojr May 31, 2017

Contributor

OK, that's what I meant.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr May 31, 2017

Contributor

I think that it's worthwhile to add description about it.

Let's work on the code first. Once we're happy about the code, we can write other stuff like documentation.

Contributor

Upabjojr commented May 31, 2017

I think that it's worthwhile to add description about it.

Let's work on the code first. Once we're happy about the code, we can write other stuff like documentation.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag May 31, 2017

Contributor

I meant this one as a global function, not a class method. Let's avoid repeating the code.

What do you think about creating class Operator and then three inherited classes?

Contributor

szymag commented May 31, 2017

I meant this one as a global function, not a class method. Let's avoid repeating the code.

What do you think about creating class Operator and then three inherited classes?

Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
Show outdated Hide outdated sympy/vector/operators.py
@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr May 31, 2017

Contributor

What do you think about creating class Operator and then three inherited classes?

Only necessary if the subclasses inherit something. In this case they don't have anything in common except for the methods of Expr. I don't think we need this.

Contributor

Upabjojr commented May 31, 2017

What do you think about creating class Operator and then three inherited classes?

Only necessary if the subclasses inherit something. In this case they don't have anything in common except for the methods of Expr. I don't think we need this.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr May 31, 2017

Contributor

Good start for the first day. I will not be available tomorrow, so see you on Friday.

Contributor

Upabjojr commented May 31, 2017

Good start for the first day. I will not be available tomorrow, so see you on Friday.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag May 31, 2017

Contributor

Thank you for your attention. I will work on a documentation and proposal of introducing Lame coefficient.
See you on Friday.

Contributor

szymag commented May 31, 2017

Thank you for your attention. I will work on a documentation and proposal of introducing Lame coefficient.
See you on Friday.

szymag and others added some commits Jun 2, 2017

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jun 2, 2017

Contributor

@szymag you should add some entry tests in sympy/core/tests/test_args.py for the newly created classes.

Contributor

Upabjojr commented Jun 2, 2017

@szymag you should add some entry tests in sympy/core/tests/test_args.py for the newly created classes.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jun 2, 2017

Contributor

Also:

  • we have now duplicate definitions of the functions (sympy.vector.functions), maybe the functions should be moved to that file and stay there.
  • lots of tests now throw a deprecation warning, they need to be changed as no coordinate system parameter should be passed to them.
Contributor

Upabjojr commented Jun 2, 2017

Also:

  • we have now duplicate definitions of the functions (sympy.vector.functions), maybe the functions should be moved to that file and stay there.
  • lots of tests now throw a deprecation warning, they need to be changed as no coordinate system parameter should be passed to them.
@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 3, 2017

Contributor

you should add some entry tests in sympy/core/tests/test_args.py for the newly created classes.

I will do that.

(sympy.vector.functions), maybe the functions should be moved to that file and stay there.

I'd like to keep functions and classes in one place. I think it has more sense. I wanted to delete rather these functions from sympy.vector.functions.

lots of tests now throw a deprecation warning, they need to be changed as no coordinate system parameter should be passed to them.

Yes, I see. I will look into tests and remove coord_sys from parameters.

I would like to also change one name of parameter, from vect to vector. Saving characters is negligible and full name looks much better.

Contributor

szymag commented Jun 3, 2017

you should add some entry tests in sympy/core/tests/test_args.py for the newly created classes.

I will do that.

(sympy.vector.functions), maybe the functions should be moved to that file and stay there.

I'd like to keep functions and classes in one place. I think it has more sense. I wanted to delete rather these functions from sympy.vector.functions.

lots of tests now throw a deprecation warning, they need to be changed as no coordinate system parameter should be passed to them.

Yes, I see. I will look into tests and remove coord_sys from parameters.

I would like to also change one name of parameter, from vect to vector. Saving characters is negligible and full name looks much better.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jun 3, 2017

Contributor

I wanted to delete rather these functions from sympy.vector.functions.

We cannot break the API, many people may be using the code with the functions in those positions, we don't want to cause widespread problems.

I would like to also change one name of parameter, from vect to vector.

I changed it into expr, because it could in principle be any expression (think about Divergence(Gradient(...)).

Contributor

Upabjojr commented Jun 3, 2017

I wanted to delete rather these functions from sympy.vector.functions.

We cannot break the API, many people may be using the code with the functions in those positions, we don't want to cause widespread problems.

I would like to also change one name of parameter, from vect to vector.

I changed it into expr, because it could in principle be any expression (think about Divergence(Gradient(...)).

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 4, 2017

Contributor

We cannot break the API, many people may be using the code with the functions in those positions, we don't want to cause widespread problems.

Yes I know. You've told me that. You also suggest creating link in sympy.vector.functions to sympy.vector.operators. I'm working on that, but simple from sympy.vector.operators import divergence, curl, gradient doesn't work. I have to find solutions.

I changed it into expr, because it could in principle be any expression (think about Divergence(Gradient(...)).

It's OK, but in functions you have still scalar and vect.

Contributor

szymag commented Jun 4, 2017

We cannot break the API, many people may be using the code with the functions in those positions, we don't want to cause widespread problems.

Yes I know. You've told me that. You also suggest creating link in sympy.vector.functions to sympy.vector.operators. I'm working on that, but simple from sympy.vector.operators import divergence, curl, gradient doesn't work. I have to find solutions.

I changed it into expr, because it could in principle be any expression (think about Divergence(Gradient(...)).

It's OK, but in functions you have still scalar and vect.

szymag added some commits Jun 6, 2017

Remove unnecessary things.
Remove coord_sys parameter from test and doctest,
remove old function in vector/functions,
apply PEP8 in in one file with test.
Add tests to core/tests/tests_args.py.
Change parameter vect to vectors in operators
functions.
@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 6, 2017

Contributor

There should be still one warning due to coord_sys parameter. I must leave explicit information about coordinate system in one test:

from sympy.vector.vector import Vector
from sympy.vector.coordsysrect import CoordSysCartesian

C = CoordSySCartesian('C')
curl(Vector.zero, C)

Vector.zero has no information about coordinate system, because it's created independently. I think it's topic for another PR.

Contributor

szymag commented Jun 6, 2017

There should be still one warning due to coord_sys parameter. I must leave explicit information about coordinate system in one test:

from sympy.vector.vector import Vector
from sympy.vector.coordsysrect import CoordSysCartesian

C = CoordSySCartesian('C')
curl(Vector.zero, C)

Vector.zero has no information about coordinate system, because it's created independently. I think it's topic for another PR.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jun 6, 2017

Contributor

We need to fix it somehow, otherwise I believe the tests will fail. Maybe a temporary if?

Contributor

Upabjojr commented Jun 6, 2017

We need to fix it somehow, otherwise I believe the tests will fail. Maybe a temporary if?

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 6, 2017

Contributor

We need to fix it somehow, otherwise I believe the tests will fail. Maybe a temporary if?

I'm also thinking about it.
For me, it seems to be quite hard problem to solve. I'd say that we should change VectorZero class, but it breaks API.

Contributor

szymag commented Jun 6, 2017

We need to fix it somehow, otherwise I believe the tests will fail. Maybe a temporary if?

I'm also thinking about it.
For me, it seems to be quite hard problem to solve. I'd say that we should change VectorZero class, but it breaks API.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jun 6, 2017

Contributor

Let's have a quick edit for this, then merge. The vector module is a bit of a mess and some code cleanup is wished.

Maybe you could add

if len(expr.free_symbols) == 0:
    return S.Zero
Contributor

Upabjojr commented Jun 6, 2017

Let's have a quick edit for this, then merge. The vector module is a bit of a mess and some code cleanup is wished.

Maybe you could add

if len(expr.free_symbols) == 0:
    return S.Zero
@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jun 6, 2017

Contributor

I would also like to get rid of the horrible way that VectorAdd and VectorMul and handled. In my opinion, those classes are useless.

Contributor

Upabjojr commented Jun 6, 2017

I would also like to get rid of the horrible way that VectorAdd and VectorMul and handled. In my opinion, those classes are useless.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 6, 2017

Contributor

I would also like to get rid of the horrible way that VectorAdd and VectorMul and handled. In my opinion, those classes are useless.

We can work on it in the next step. I believe it will be easier later.

Contributor

szymag commented Jun 6, 2017

I would also like to get rid of the horrible way that VectorAdd and VectorMul and handled. In my opinion, those classes are useless.

We can work on it in the next step. I believe it will be easier later.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jun 6, 2017

Contributor

Yes, let's focus on the main topics of the GSoC first.

Contributor

Upabjojr commented Jun 6, 2017

Yes, let's focus on the main topics of the GSoC first.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 6, 2017

Contributor

S.zero doesn't work. I get error:

AttributeError: 'Zero' object has no attribute 'delop'

I should return coordinate system.

Contributor

szymag commented Jun 6, 2017

S.zero doesn't work. I get error:

AttributeError: 'Zero' object has no attribute 'delop'

I should return coordinate system.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 6, 2017

Contributor

I put it in _get_coord_sys_from_expr but now I think you proposed to put it in curl, yes?

Contributor

szymag commented Jun 6, 2017

I put it in _get_coord_sys_from_expr but now I think you proposed to put it in curl, yes?

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 6, 2017

Contributor

I think it should pass now. I had to add another exception for 0. We can put scalar field as int in one case, when is equal to 0.

Contributor

szymag commented Jun 6, 2017

I think it should pass now. I had to add another exception for 0. We can put scalar field as int in one case, when is equal to 0.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 7, 2017

Contributor

Do you like changes in operators.py? I have to add if statement in _get_coord_sys_from_expr but also in gradient, curl and divergence.

Contributor

szymag commented Jun 7, 2017

Do you like changes in operators.py? I have to add if statement in _get_coord_sys_from_expr but also in gradient, curl and divergence.

@szymag szymag changed the title from [WIP] Create operators.py file to Create operators.py file Jun 7, 2017

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jun 7, 2017

Contributor

When we merged this PR, I'd like to remove coord_sys from functions in functions.py file, like scalar_potential_difference(field, coord_sys, point1, point2).
It's easy task.

Contributor

szymag commented Jun 7, 2017

When we merged this PR, I'd like to remove coord_sys from functions in functions.py file, like scalar_potential_difference(field, coord_sys, point1, point2).
It's easy task.

@Upabjojr Upabjojr merged commit ecf4de8 into sympy:master Jun 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@szymag szymag deleted the szymag:unevaluated_expressions branch Jun 12, 2017

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