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

Reconstruction of directional derivative #12991

Merged
merged 2 commits into from Jul 20, 2017

Conversation

Projects
None yet
2 participants
@szymag
Contributor

szymag commented Jul 18, 2017

Calculation are moved from Vector class to separate functions.
Update comments and tests.

The problem are arguments. In directional_derivative function which was added recently (#12417) not only usage of this function is different from Vector class (see #12969) but also the order.

The problematic code are already in 1.1 version. Should we add deprecation?

Move calculation of directional derivative.
Calculation are moved from vector class to separate functions. Update
comments and tests.
@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 18, 2017

Contributor

ping @Upabjojr

Contributor

szymag commented Jul 18, 2017

ping @Upabjojr

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 18, 2017

Contributor

@szymag we cannot just break the API by changing the order of parameters to a function.

Contributor

Upabjojr commented Jul 18, 2017

@szymag we cannot just break the API by changing the order of parameters to a function.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 18, 2017

Contributor

(apart from that, this looks good)

Contributor

Upabjojr commented Jul 18, 2017

(apart from that, this looks good)

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 19, 2017

Contributor

@Upabjojr, I'd like to add class Directional_derivative in separate PR, so if you will think that current changes are OK, please merge it.

Contributor

szymag commented Jul 19, 2017

@Upabjojr, I'd like to add class Directional_derivative in separate PR, so if you will think that current changes are OK, please merge it.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 19, 2017

Contributor

Sorry, we have a compatibility break. SymPy 1.1 supports the opposite order, end-users are likely to start using it like that. We cannot invert the parameter order in SymPy 1.2, otherwise the user community will start complaining about the changes.

Contributor

Upabjojr commented Jul 19, 2017

Sorry, we have a compatibility break. SymPy 1.1 supports the opposite order, end-users are likely to start using it like that. We cannot invert the parameter order in SymPy 1.2, otherwise the user community will start complaining about the changes.

@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 19, 2017

Contributor

Maybe I wrote this not precisely. I turn my order so I leave it as it was.
Now I have only one doubt corresponding to name, scalar or field?

Contributor

szymag commented Jul 19, 2017

Maybe I wrote this not precisely. I turn my order so I leave it as it was.
Now I have only one doubt corresponding to name, scalar or field?

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 19, 2017

Contributor

My idea is to keep the code as close as it is in master (so keeping the same names). Only change the names if they are wrong.

Contributor

Upabjojr commented Jul 19, 2017

My idea is to keep the code as close as it is in master (so keeping the same names). Only change the names if they are wrong.

Turn order of parameters in directional_derivative.
Now, te order is the same as it was previously.
@szymag

This comment has been minimized.

Show comment
Hide comment
@szymag

szymag Jul 19, 2017

Contributor

So I leave field argument because scalar is wrong.

Contributor

szymag commented Jul 19, 2017

So I leave field argument because scalar is wrong.

@Upabjojr

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 19, 2017

Contributor

So I leave field argument because scalar is wrong.

OK.

Contributor

Upabjojr commented Jul 19, 2017

So I leave field argument because scalar is wrong.

OK.

@szymag szymag changed the title from [WIP] Reconstruction of directional derivative to Reconstruction of directional derivative Jul 20, 2017

@Upabjojr Upabjojr merged commit e95969f into sympy:master Jul 20, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@Upabjojr

Upabjojr Jul 20, 2017

Contributor

Oh, didn't realize that this PR was finished.

Contributor

Upabjojr commented Jul 20, 2017

Oh, didn't realize that this PR was finished.

@szymag szymag deleted the szymag:directional_derivative branch Aug 23, 2017

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