Tensor module: refactory to allow mixing PartialDerivative and TensAdd

[Upabjojr]

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

• tensor
  • Tensor module: refactory to allow mixing PartialDerivative and TensAdd.

[sympy-bot]

✅

Hi, I am the SymPy bot (v149). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• tensor
  • Tensor module: refactory to allow mixing PartialDerivative and TensAdd. (#18224 by @Upabjojr)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* tensor
  * Tensor module: refactory to allow mixing PartialDerivative and TensAdd.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[oscarbenjamin]

I don't know this part of the codebase well but the PR looks fine to me.

I notice that you have introduced type annotations in the comments. Is that the recommended way to use mypy?

[Upabjojr]

I notice that you have introduced type annotations in the comments. Is that the recommended way to use mypy?

I got them from mypy's documentation:
https://mypy.readthedocs.io/en/latest/python2.html

I didn't try mypy yet, but PyCharm is already compatible with type annotation and appears to work fine (i.e. with type annotations you get correct auto-completion, jumps to definitions, etc...).

@oscarbenjamin I have also changed the metaclass of sympy.tensor.tensor in order to support abstract methods (that is, raise an exception if you instantiate a subclass that has not implemented all abstract methods). This could be of general usage to the whole of SymPy, maybe Expr should also have ABCMeta as metaclass?

[oscarbenjamin]

This could be of general usage to the whole of SymPy, maybe Expr should also have ABCMeta as metaclass?

I think there are certainly other places where it could be useful. It would be better if maybe MatrixBase or MatrixExpr used this and actually defined the minimal API that any concrete matrix class should provide.

For Expr this is tricky because it has way too many methods. Which would you expect a subclass to provide? If you do git grep Expr | grep class you can see a lot of subclasses.

More generally I wonder about the ManagedProperties metaclass. It defines assumptions that only really make sense for numbers but is used by all of Basic including Set, Boolean, MatrixExpr, etc. Are things like is_prime meaningful for tensors?

[Upabjojr]

This could be of general usage to the whole of SymPy, maybe Expr should also have ABCMeta as metaclass?

I think there are certainly other places where it could be useful. It would be better if maybe MatrixBase or MatrixExpr used this and actually defined the minimal API that any concrete matrix class should provide.

Yes, definitely. All subclasses of MatrixExpr need to implement .shape, that could be enforced with an abstract method.

For Expr this is tricky because it has way too many methods. Which would you expect a subclass to provide? If you do git grep Expr | grep class you can see a lot of subclasses.

Maybe we can simply move _TensorMetaclass to sympy.core and give it a more general name.

More generally I wonder about the ManagedProperties metaclass. It defines assumptions that only really make sense for numbers but is used by all of Basic including Set, Boolean, MatrixExpr, etc. Are things like is_prime meaningful for tensors?

The purpose of the new-style assumptions was getting rid of the metaclasses, IIRC. ManagedProperties has been a bad design choice, but it's really hard to get rid of it now, as it's used everywhere in SymPy.

[codecov]

Codecov Report

Merging #18224 into master will increase coverage by 0.016%.
The diff coverage is 89.411%.

@@              Coverage Diff              @@
##            master    #18224       +/-   ##
=============================================
+ Coverage   74.938%   74.954%   +0.016%     
=============================================
  Files          642       642               
  Lines       167166    167222       +56     
  Branches     39340     39347        +7     
=============================================
+ Hits        125271    125341       +70     
+ Misses       36362     36334       -28     
- Partials      5533      5547       +14

[oscarbenjamin]

More generally I wonder about the ManagedProperties metaclass. It defines assumptions that only really make sense for numbers but is used by all of Basic including Set, Boolean, MatrixExpr, etc. Are things like is_prime meaningful for tensors?

The purpose of the new-style assumptions was getting rid of the metaclasses, IIRC. ManagedProperties has been a bad design choice, but it's really hard to get rid of it now, as it's used everywhere in SymPy.

I don't know that there is any agreed plan for new vs old assumptions any more. It seems to me that the new assumptions can not replace the old because they are built on top of the core.

I think that ManagedProperties is fine except that it should be scoped so that there is one set of assumptions for "numbers" and another for "matrices" or "sets" etc. I would propose that each class should have an associated "klass" like number/boolean/set/matrix/tuple etc. The defined assumption properties and implication rules should be connected to the "klass" rather than defined globally for all of Basic.

For Expr this is tricky because it has way too many methods. Which would you expect a subclass to provide? If you do git grep Expr | grep class you can see a lot of subclasses.

Maybe we can simply move _TensorMetaclass to sympy.core and give it a more general name.

We could do but the question is what would be the equivalent of TensExpr for Expr and what abstract methods would it define? Those methods would then have to be defined for all subclasses of Expr (not backward compatible for any down-stream user who has subclassed Expr).

Oh wait: Do you just mean that Basic should have ABCMeta as a metaclass so that Basic subclasses can use abstractmethod without needing to include the metaclass explicitly? If so then yes I think that probably makes sense.

[Upabjojr]

I think there's a fundamental design issue: assumptions should not be bound to a class, they should apply to any expression. The current API x.is_<assumption> only considers the possibility of associating an assumption to symbols. There is no way to define assumptions on expressions (for example, the assumption that the sum of two variables is positive).

You cannot go on with a metaclass, assumptions should really be separate from Expr objects.

[Upabjojr]

Oh wait: Do you just mean that Basic should have ABCMeta as a metaclass so that Basic subclasses can use abstractmethod without needing to include the metaclass explicitly? If so then yes I think that probably makes sense.

Yes, now you are free to use @abstractmethod, but there is no enforcement on it (so you can still instantiate abstract classes without problem). If ManagedProperties were to inherit ABCMeta, you won't be able to instantiate abstract classes.

The only question is whether subclassing ABCMeta in ManagedProperties could impact on SymPy's performance. Maybe it's better to create a subclass of both ManagedProperties and ABCMeta and use it only when needed (like in this PR).

[oscarbenjamin]

The only question is whether subclassing ABCMeta in ManagedProperties could impact on SymPy's performance.

If you push that as a change here we can see the result of running the benchmarks on Travis

[Upabjojr]

If you push that as a change here we can see the result of running the benchmarks on Travis

Do you think you can trust Travis? I got the feeling that their CPUs are quite different every time you run the tests.

[oscarbenjamin]

If you push that as a change here we can see the result of running the benchmarks on Travis

Do you think you can trust Travis? I got the feeling that their CPUs are quite different every time you run the tests.

It compares master with the PR in the same job and seems to be comparable. You can see the benchmarks for this PR here:
https://travis-ci.org/sympy/sympy/jobs/632648565?utm_medium=notification&utm_source=github_status
(Look down to the bottom for the results.)

You can also run the benchmarks yourself.

I'm not sure why the metaclass would give a significant slow-down. I presume the additional processing happens once per-class rather than once per-instance so it would be a small slowdown on import.

[Upabjojr]

@oscarbenjamin let's see how this compares to #18228

[Upabjojr]

Merging this PR as the other one is slowing down SymPy.