Opened 15 months ago

Last modified 3 months ago

#30307 new enhancement

Refactor Components into parent & element

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: linear algebra Keywords:
Cc: gh-mjungmath, egourgoulhon, tscrim, gh-honglizhaobob Merged in:
Authors: Michael Jung, Matthias Koeppe Reviewers:
Report Upstream: N/A Work issues:
Branch: public/30307 (Commits, GitHub, GitLab) Commit: e5a81f253cea4e72c930e6d4a0fb0cc217b14f0e
Dependencies: #31904, #29619 Stopgaps:

Status badges

Description (last modified by mkoeppe)

Currently every Components object has lots of metadata attributes in addition to the actual data dictionary in ._comp.

If one has many different Components objects with the same symmetry metadata, we can reduce storage space as follows.

We create new classes CompParent, CompParentWithSym, ..., which store the symmetry attributes and become UniqueRepresentation. We make Components objects elements of these parents.

The parents will also have index iterator methods.

Data associated with symmetries and dimensions, computed currently each time for each CompWithSym object in methods such as __init__, __add__, trace, ... can be precomputed and cached in the parent using @cached_method in the parent class).

This will make the code in #30229 (subspaces of tensor with symmetries) more elegant because it no longer needs a dummy Components object to represent the symmetry but rather a CompParent object.

See also:

  • #32029 Action of a sympy TensorSymmetry

Change History (103)

comment:1 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:2 Changed 9 months ago by mkoeppe

Help with implementing this would be very welcome!

comment:3 Changed 9 months ago by gh-mjungmath

That's a nice idea, +1!

Data associated with symmetries, computed currently each time for each CompWithSym object in methods such as __init__, __add__, trace, ... can then be precomputed and cached in the parent (for example using @cached_method in the parent class).

I think it would be a good idea to cache results from trace, contract, ... as well.

The most annoying thing with this ticket will most likely be the docstring that has to be adapted...is there a good way to use search&replace?

comment:4 Changed 9 months ago by gh-mjungmath

Currently, the index generators and manipulators take (mostly) lists and can therefore not be cached. To use the caching most efficiently, I suggest we switch entirely to tuples.

Last edited 9 months ago by gh-mjungmath (previous) (diff)

comment:5 Changed 9 months ago by gh-mjungmath

  • Branch set to public/30307

comment:6 Changed 9 months ago by git

  • Commit set to 943f9ea4eb95d1e5b162ef88194799f1711d95c1

Branch pushed to git repo; I updated commit sha1. New commits:

fec4b06fix sage -b after 30622
dff846cgo through make/build/install
943f9eaTrac #30307: CompParent and CompParentWithSym

comment:7 follow-up: Changed 9 months ago by gh-mjungmath

Before I go on with this ticket, could you please take a look whether this meets your rough idea?

Is anyone willing to work on the docstrings...? Perhaps there's an efficient way to do this?

comment:8 Changed 9 months ago by git

  • Commit changed from 943f9ea4eb95d1e5b162ef88194799f1711d95c1 to a2514b6e70a05dbb354b535d4acbfdde4f55fdb7

Branch pushed to git repo; I updated commit sha1. New commits:

a2514b6Trac #30307: no symmetries -> CompParent without symmetries

comment:9 in reply to: ↑ 7 ; follow-up: Changed 9 months ago by mkoeppe

Replying to gh-mjungmath:

Before I go on with this ticket, could you please take a look whether this meets your rough idea?

Yes, this is going in the direction that I had in mind.

Some more of the normalization happening in CompParentWithSym.__init__ should probably be moved to __classcall_private__

comment:10 follow-up: Changed 9 months ago by gh-mjungmath

What would be a proper category for the parent?

comment:11 in reply to: ↑ 9 Changed 9 months ago by gh-mjungmath

Replying to mkoeppe:

Some more of the normalization happening in CompParentWithSym.__init__ should probably be moved to __classcall_private__

For example?

comment:12 Changed 9 months ago by mkoeppe

For example the order of the component indices does not matter

comment:13 in reply to: ↑ 10 Changed 9 months ago by egourgoulhon

Replying to gh-mjungmath:

What would be a proper category for the parent?

Take the following with a grain of salt (this is just a rough/naive thought): It seems to me that the current proposal is a kind of abuse of Sage's parent/element scheme, for CompParent does not correspond to a "genuine" mathematical object (hence maybe your question...). In particular, it does not know about the ring on which the components are based. I do not deny that a reorganization of Components would be a good thing, but maybe at the class level, not at the parent/element level, the issue here being mostly the storage of metadata.

comment:14 follow-up: Changed 9 months ago by mkoeppe

Just use the category of sets. I don't think this is an abuse.

Think of an instance of CompParent as the set of all possible Components instances that have the specified symmetry.

By using the parent/element framework, you will get coercion for free - so elements with a coarser symmetry will be in a finer parent.

comment:15 in reply to: ↑ 14 Changed 9 months ago by gh-mjungmath

Replying to mkoeppe:

For example the order of the component indices does not matter

Right!

Replying to mkoeppe:

Just use the category of sets. I don't think this is an abuse.

Think of an instance of CompParent as the set of all possible Components instances that have the specified symmetry.

By using the parent/element framework, you will get coercion for free - so elements with a coarser symmetry will be in a finer parent.

For me, it doesn't feel like an abuse either. Besides, I am sure you can make that notion of compontents mathematically rigorous. But I am not sure whether this becomes a well-defined set then... What about the category of objects?

comment:16 Changed 9 months ago by gh-mjungmath

Alright, I make progress here. I changed only the backend such that most doctests should still pass, and the module can be used exactly as before. The elementary examples already passed.

I'll push my branch tomorrow.

I am looking forward to some benchmarks as soon as this branch is ready. :)

comment:17 follow-ups: Changed 9 months ago by tscrim

I can understand why this might seem like an abuse because it is a set of objects, but by extension, all of the combinatorial objects would be an abuse as well. So even though we will not be putting an extra mathematical structure on the components, this is still a valid use of the framework because there is a set of objects (the parent) and the individual objects (the elements).

Something to consider, however, is a potential speed penalty for the extra levels of initialization. This can be somewhat mitigated by using Cython, but performance regression testing is warranted here.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 9 months ago by gh-mjungmath

Replying to tscrim:

This can be somewhat mitigated by using Cython, but performance regression testing is warranted here.

How so?

comment:19 in reply to: ↑ 18 Changed 9 months ago by tscrim

Replying to gh-mjungmath:

Replying to tscrim:

This can be somewhat mitigated by using Cython, but performance regression testing is warranted here.

How so?

Because Cython is faster than Python, including potential benefits from direct C-level function calls.

comment:20 Changed 9 months ago by gh-mjungmath

Yes, that's clear. I meant, how to implement? Make CompParent and Components both simply extension types?

Would cdpefing the symmetry related functions cause a speedup btw?

Since this involves only integers, i.e. struct types, one could even try to Cythonize these completely.

comment:21 Changed 9 months ago by git

  • Commit changed from a2514b6e70a05dbb354b535d4acbfdde4f55fdb7 to fc22d5dd93320730cf97acfb426dedc6cba037c2

Branch pushed to git repo; I updated commit sha1. New commits:

fc22d5dTrac 30307: Cythonize, restructuring, factory method to resemble old behavior

comment:22 in reply to: ↑ 17 Changed 9 months ago by egourgoulhon

Replying to tscrim:

I can understand why this might seem like an abuse because it is a set of objects, but by extension, all of the combinatorial objects would be an abuse as well. So even though we will not be putting an extra mathematical structure on the components, this is still a valid use of the framework because there is a set of objects (the parent) and the individual objects (the elements).

Thank you Matthias, Michael and Travis for your replies. I'm convinced now that parent/element is a good approach here (I was bothered by the lack of algebraic structure of the parent), especially for symmetry-based coercions.

Something to consider, however, is a potential speed penalty for the extra levels of initialization. This can be somewhat mitigated by using Cython, but performance regression testing is warranted here.

Certainly!


New commits:

fc22d5dTrac 30307: Cythonize, restructuring, factory method to resemble old behavior

comment:23 Changed 9 months ago by git

  • Commit changed from fc22d5dd93320730cf97acfb426dedc6cba037c2 to 27bc9b1ddd376da516228b8c8144c6139376f061

Branch pushed to git repo; I updated commit sha1. New commits:

27bc9b1Trac #30307: pdx file + __init__.py removed

comment:24 follow-up: Changed 9 months ago by egourgoulhon

A possibly relevant ticket, about enhancing symmetries of components beyond the currently implemented ones: #28813


New commits:

27bc9b1Trac #30307: pdx file + __init__.py removed

comment:25 Changed 9 months ago by gh-mjungmath

I am sorry for the mess. However, this is my rough approach. I separated elements and parents, and established factory methods to recover the old behavior (backwards compatibility and less work on the docstrings).

I am open to suggestions how to separate the symmetry code apart. Some symmetry treatments are coupled to the element for optimization (e.g. antisymmetrize to determine zero more quickly).

This code is too big for me to do it alone. I'd appreciate some help here. I hope my first approach is of some use for you.

comment:26 Changed 9 months ago by egourgoulhon

A concern about cythonizing the whole thing: the Python debugger cannot be used in Cython parts of codes, which is a pity, IMHO. So if Cython does not bring any significant performance improvement, we are loosing more than we gain.

comment:27 follow-up: Changed 9 months ago by egourgoulhon

Also shouldn't one perform a single task in this ticket, namely refactor Components into parent/element, and leave cythonization to another ticket?

comment:28 in reply to: ↑ 27 Changed 9 months ago by gh-mjungmath

Replying to egourgoulhon:

Also shouldn't one perform a single task in this ticket, namely refactor Components into parent/element, and leave cythonization to another ticket?

That makes sense. In the next step one could try to implement #28813, and perhaps even use Cython to do so.

Besides, I just learned that one could increase the init speed with Python 3 using __slots__. Maybe that's what we should do for now.

Still, I don't know how we should separate the symmetry code properly. My idea so far was to let the parent do the symmetrizing work and return the parent with the corresponding symmetries, and construct an element from it, then assign the components. But this doesn't work so well when the element itself such as in antisymmetrize is taken into account.

Alternatively, it would be nice do define maps (morphisms) between parents doing that work and cache those morphisms. But that is something I don't know how to do in a nice and proper way. For example, if one antisymmetrizes in the variables where the components are already symmetric, it's simply the "zero morphism".

Last edited 9 months ago by gh-mjungmath (previous) (diff)

comment:29 Changed 9 months ago by gh-mjungmath

Nevertheless, I think that the rough refactoring into comp_element and comp_parent (and comp to recover the old behavior) is already a good way to go.

comment:30 in reply to: ↑ 24 Changed 9 months ago by mkoeppe

Replying to egourgoulhon:

A possibly relevant ticket, about enhancing symmetries of components beyond the currently implemented ones: #28813

I agree that it would be a good idea to keep possible generalizations in mind while doing this refactoring. (Also #30276)

comment:31 Changed 9 months ago by gh-mjungmath

Definitely! So, do we have a roadmap?

comment:32 Changed 9 months ago by mkoeppe

This is looking nice already, and I agree that this ticket should do a Python implementation only. I would hope that there are already performance gains by caching.

comment:33 Changed 9 months ago by gh-mjungmath

@Mattihas: What do you say about the morphism idea in comment:27?

@Travis: What about using __slots__ in this ticket instead of Cythonizing?

comment:34 follow-up: Changed 9 months ago by mkoeppe

Regrading the morphisms: These are exactly the reduce, retract, lift maps that we discussed in #30229 - just on the level of components rather than modules.

comment:35 in reply to: ↑ 34 Changed 9 months ago by gh-mjungmath

Replying to mkoeppe:

Regrading the morphisms: These are exactly the reduce, retract, lift maps that we discussed in #30229 - just on the level of components rather than modules.

Mh...but I suggest more than 3 morphisms. I don't know what you mean, sorry.

Can I take it as a "yes, what a wonderful idea!"? :P

Last edited 9 months ago by gh-mjungmath (previous) (diff)

comment:36 Changed 9 months ago by mkoeppe

Yes, there are many maps:

  • For a CompParent with a coarser symmetry, there is an injection (lift map) to any CompParent with a finer symmetry.

etc. (And yes, it's a wonderful idea.)

comment:37 Changed 9 months ago by tscrim

I don't expect __slots__ to provide much benefit to speed as from what I am reading, it is more useful for memory usage. The main thing is to turn the file into a .pyx file with a .pxd header for declarations. The element class should be a cpdef with the main parameters declared and given explicit typing. These are relatively easy things to do provided you don't have multiple inheritance in the element classes (the parent can remain normal Python classes in a pyx file).

IMO, you also don't loose too much with converting to Cython with debugging because you still get a lot with error tracebacks. The biggest thing I have found lost is the ease of profiling to find bottlenecks. However, testing with practical examples can get around that a bit, and there is a tool to profile Cython code IIRC. So I generally see this as a smaller trade-off.

comment:38 Changed 9 months ago by gh-mjungmath

Well, __slots__ makes initializations faster because attribute access is significantly faster (up to 30%). Memory is just an additional benefit.

Even if we turn Components into a cpdef, I think we still gain a good performance boost with __slots__ which can sum up quickly with recurrent use of _comp.

Last edited 9 months ago by gh-mjungmath (previous) (diff)

comment:39 Changed 9 months ago by gh-mjungmath

To use morphisms, we need a new category (say Category of collections of abstract components) and a homset, right?

comment:40 Changed 9 months ago by gh-mjungmath

Btw, I am not convinced that cpdefing the class makes initializations faster, I'd bet on the contrary. When I understand it correctly, cpdef creates two classes in the background, an extension type and a usual Python class. Since we don't expect a benefit during the lifetime of an instance, it should even slowdown things. But maybe I just got things wrong here.

Last edited 9 months ago by gh-mjungmath (previous) (diff)

comment:41 Changed 9 months ago by gh-mjungmath

Last edited 9 months ago by gh-mjungmath (previous) (diff)

comment:42 follow-up: Changed 9 months ago by tscrim

What are you talking about cpdefing a class? You only do that to functions AFAIK. I am still not 100% convinced by the __slots__ approach, more so because it makes it (slightly) harder to switch from a Python class to an extension class IIRC.

I don't see why we need a new category. We don't need to be so heavy-handed with the approach. Having the homset be in the category of (enumerated?) sets is sufficient IMO.

comment:43 follow-up: Changed 9 months ago by tscrim

The main benefit with Cython and extension classes is being able to strongly type things to have as many C level function calls and code as possible.

comment:44 in reply to: ↑ 42 ; follow-up: Changed 9 months ago by gh-mjungmath

Replying to tscrim:

I don't see why we need a new category. We don't need to be so heavy-handed with the approach. Having the homset be in the category of (enumerated?) sets is sufficient IMO.

Mathematically, I am still not convinced that our parents constitute sets. Their elements run over all possible rings (and "frames"), which is not a set either.

comment:45 in reply to: ↑ 43 Changed 9 months ago by gh-mjungmath

Replying to tscrim:

The main benefit with Cython and extension classes is being able to strongly type things to have as many C level function calls and code as possible.

Indeed. That needs a thorough modification of the current code. The indices checks use mostly lists and Python sets. The components are stored as dictionaries which cannot be strongly typed. I agree, that should eventually be done, however I propose that's something for another ticket.

comment:46 Changed 9 months ago by gh-mjungmath

For now, we can use __slots__ for a slight speedup and remove it again when we Cythonize. That's what I meant.

comment:47 Changed 9 months ago by tscrim

I don't really like this partial measure. I wouldn't do it and just keep it pure Python until you actually decide to make it Cython.

Why is the ring associated with the element and not the parent? In general, why is all of these attributes copied from the parent? Is the extra level of indirection that slow? You can just call self._parent in the Cython code.

Because of how your current implementation is done, you are not going to benefit from coercions. Nor do you seem to be using anything in a category. Thus, I would actually weaken things and not use Sage's Parent/Element? classes. Instead, I would just mimic them. Perhaps I am misunderstanding how you plan to apply these.

There is still a lot of you things you can do to tell Cython to make things be strongly typed even if they are extracted from lists/sets/dicts, such as type-casting.

comment:48 follow-up: Changed 9 months ago by gh-mjungmath

Please don't look at the details yet. The code is far away from being finished. Coercions come, attributes will be removed (it's just to make parts of the code already debuggable).

Why has the parent no ring? Because the symmetries and indices simply do not depend on it. On the level of indices and symmetries we have: different ring -> same data -> same instance -> more effective storage.

comment:49 in reply to: ↑ 48 ; follow-up: Changed 9 months ago by tscrim

Replying to gh-mjungmath:

Why has the parent no ring? Because the symmetries and indices simply do not depend on it. On the level of indices and symmetries we have: different ring -> same data -> same instance -> more effective storage.

You're passing around a pointer to the ring in a lot of the elements, which is storage. Since I doubt the ring will change much, I don't think you gain much. Plus you have to pass more around, which makes maintenance harder and can come with performance impacts.

comment:50 in reply to: ↑ 44 Changed 9 months ago by egourgoulhon

Replying to gh-mjungmath:

Mathematically, I am still not convinced that our parents constitute sets. Their elements run over all possible rings (and "frames"), which is not a set either.

I've also the feeling that this kind of parents cannot be sets in the meaning of standard set theory.

comment:51 in reply to: ↑ 49 Changed 9 months ago by gh-mjungmath

Last edited 9 months ago by gh-mjungmath (previous) (diff)

comment:52 follow-up: Changed 9 months ago by gh-mjungmath

Alright, probably you're right, Travis (as always). :P

But I'm still convinced that __slots__ is a good thing for now. It will take a while until we have Cythonized everything.

Even if we delegate the ring to the parent, they are still no sets. The "frame" is still an object in the category of finite sets (with length n).

comment:53 in reply to: ↑ 52 Changed 9 months ago by tscrim

Replying to gh-mjungmath:

But I'm still convinced that __slots__ is a good thing for now. It will take a while until we have Cythonized everything.

I am not, but I don't have the same stake in the code as you. A little experimentation is not a bad thing either.

comment:54 Changed 9 months ago by gh-mjungmath

Mh. Matthias, Eric, what are your opinions on that __slot__ matter?

comment:55 follow-up: Changed 9 months ago by mkoeppe

It's a further optimization and I think it should be tried in a follow-up, not this ticket

comment:56 in reply to: ↑ 55 Changed 9 months ago by egourgoulhon

Replying to mkoeppe:

It's a further optimization and I think it should be tried in a follow-up, not this ticket

+1

comment:57 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:58 follow-up: Changed 8 months ago by gh-mjungmath

Alright, let's do that optimization in a follow-up.

However, I am still concerned about CompParent being a proper class, which means that the collection of all those parents is no class at all and hence no category in terms of strict category theory.

Any ideas how to circumvent this matter?

comment:59 in reply to: ↑ 58 Changed 8 months ago by tscrim

Replying to gh-mjungmath:

However, I am still concerned about CompParent being a proper class, which means that the collection of all those parents is no class at all and hence no category in terms of strict category theory.

Any ideas how to circumvent this matter?

Stop worrying about it. We can have slight abuses in Sage (see RR being a field), and this is also something that is a very technical point that is occurring below what most users will see.

comment:60 Changed 7 months ago by gh-mjungmath

This task seems nothing that can be done in just one day. I would appreciate a little help and direction here. Especially w.r.t. #30276 and #28813. It would be a shame to put a lot of work in here if we had to refactor it again to achieve #30276 and #28813.

comment:61 Changed 7 months ago by mkoeppe

I'll return to it after the 9.3 release

comment:62 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:63 Changed 4 months ago by gh-honglizhaobob

  • Cc gh-honglizhaobob added

comment:64 Changed 4 months ago by git

  • Commit changed from 27bc9b1ddd376da516228b8c8144c6139376f061 to 19f0a222e96da2206ec666a3008f7df51be96215

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

64a17eeTrac #30307: CompParent and CompParentWithSym
2186b66Trac #30307: no symmetries -> CompParent without symmetries
08f3b7bTrac 30307: Cythonize, restructuring, factory method to resemble old behavior
7c2a7c5Trac #30307: pdx file + __init__.py removed
19f0a22sage.tensor.modules.comp_element: Back to pure Python

comment:65 Changed 4 months ago by mkoeppe

I have rebased the branch on top of the current develop.

To keep the ticket more focused, I have undone the Cythonization.

comment:66 Changed 4 months ago by mkoeppe

I am going to move start_index from the parents to the elements because the symmetries have nothing to do with it.

comment:67 Changed 4 months ago by mkoeppe

Same also for _dim.

Method index_generator will take a list of range objects - this will provide a generalization for the case of tensors between modules of differing ranks

comment:68 Changed 4 months ago by git

  • Commit changed from 19f0a222e96da2206ec666a3008f7df51be96215 to 75aed16b63e8fee4e2ffbda9cd2a5d6b43fd52d5

Branch pushed to git repo; I updated commit sha1. New commits:

75aed16WIP: Remove dim and start_index from CompParent

comment:69 Changed 4 months ago by git

  • Commit changed from 75aed16b63e8fee4e2ffbda9cd2a5d6b43fd52d5 to 14a070f810a61a162908798f8869c7d1310ca76c

Branch pushed to git repo; I updated commit sha1. New commits:

14a070fWIP: CompParent

comment:70 Changed 4 months ago by git

  • Commit changed from 14a070f810a61a162908798f8869c7d1310ca76c to f6bde0a9f9cfd69254aa0ca1ba8b0ffc04b77677

Branch pushed to git repo; I updated commit sha1. New commits:

f6bde0aFinish comp_parent.py

comment:71 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:72 Changed 4 months ago by git

  • Commit changed from f6bde0a9f9cfd69254aa0ca1ba8b0ffc04b77677 to cabcd49408c1d08be982b5c36d2b1deec779e4f5

Branch pushed to git repo; I updated commit sha1. New commits:

cabcd49src/sage/tensor/modules/comp*.py: Mostly working

comment:73 Changed 4 months ago by git

  • Commit changed from cabcd49408c1d08be982b5c36d2b1deec779e4f5 to d8a7208977f3433f9a5f107f30326755cca71e9c

Branch pushed to git repo; I updated commit sha1. New commits:

d8a7208sage.tensor.modules.comp_element_dict: Split out from sage.tensor.modules.comp_element

comment:74 Changed 4 months ago by mkoeppe

Started to prepare the classes for multiple backend implementations.

comment:75 Changed 4 months ago by git

  • Commit changed from d8a7208977f3433f9a5f107f30326755cca71e9c to 5395aa37acf0d26543fa7c4d0f3950f23403684f

Branch pushed to git repo; I updated commit sha1. New commits:

5395aa3Update doctest output for trivial output changes

comment:76 Changed 4 months ago by git

  • Commit changed from 5395aa37acf0d26543fa7c4d0f3950f23403684f to 7d6ffede2fd7f191bcc03d506fa9b796304f592f

Branch pushed to git repo; I updated commit sha1. New commits:

e772f70Fix symmetrization bug
7d6ffedUpdate doctest output for cases where the code now detects full symmetry

comment:77 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:78 Changed 4 months ago by mkoeppe

  • Dependencies set to #31904

comment:79 Changed 4 months ago by git

  • Commit changed from 7d6ffede2fd7f191bcc03d506fa9b796304f592f to eed8f46a0eb7b7d25b9a754b2aa7b6f7853a8f7e

Branch pushed to git repo; I updated commit sha1. New commits:

8455aabFix bug #31904 in pullback
aea4554#31904: Fix indentation in _pullback_chart
cfb629cMerge #31904
eed8f46DiffMap.pullback: Replace dispatching to Comp* classes by parent call

comment:80 Changed 4 months ago by git

  • Commit changed from eed8f46a0eb7b7d25b9a754b2aa7b6f7853a8f7e to 6da636ee3a3f0f615a01c22b1db61fd208dc819f

Branch pushed to git repo; I updated commit sha1. New commits:

6da636eCompParent...: Add __classcall_private__ methods

comment:81 Changed 4 months ago by git

  • Commit changed from 6da636ee3a3f0f615a01c22b1db61fd208dc819f to 9c0d7f1231d02e9e9444b413f29a67e4deffd965

Branch pushed to git repo; I updated commit sha1. New commits:

3e9bfefFreeModuleTensor: Make _sym, _antisym tuples, fix repr of Symmetric bilinear form
9c0d7f1sage.tensor.modules: Update doctest output for trivial output changes

comment:82 Changed 4 months ago by mkoeppe

I will need to revise the design a bit to accommodate different backends.

A CompParent will become a free module over a fixed ring.

Fixing the ring is necessary because numerical backends (for example for NumPy's ndarray) can only work with base ring RDF; whereas the symbolic backends (such as the existing dictionary-based implementation) is for SR and exact rings.

comment:83 Changed 4 months ago by git

  • Commit changed from 9c0d7f1231d02e9e9444b413f29a67e4deffd965 to 4408f1de7b630547afec1c052c4e236570c99de9

Branch pushed to git repo; I updated commit sha1. New commits:

e1b6341Make Components a module over 'ring'
4408f1dMake Components_base a ModuleElementWithMutability

comment:84 Changed 4 months ago by mkoeppe

I've arrived at a point where it just remains to teach the coercion system about coercions/pushouts to combine operands with differing symmetries

File "src/sage/tensor/modules/comp_element_dict.py", line 914, in sage.tensor.modules.comp_element_dict.ComponentsWithSym_dict
Failed example:
    s = a + b ; s
Exception raised:
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tensor.modules.comp_element_dict.ComponentsWithSym_dict[27]>", line 1, in <module>
        s = a + b ; s
      File "sage/structure/element.pyx", line 1232, in sage.structure.element.Element.__add__
        return coercion_model.bin_op(left, right, add)
      File "sage/structure/coerce.pyx", line 1248, in sage.structure.coerce.CoercionModel.bin_op
        raise bin_op_exception(op, x, y)
    TypeError: unsupported operand parent(s) for +: 'Parent of 2-index components over Rational Field' and 'Parent of Fully symmetric 2-index components over Rational Field'

comment:85 Changed 4 months ago by git

  • Commit changed from 4408f1de7b630547afec1c052c4e236570c99de9 to e9cb1cd84b5b65d97e86fc1680d2011c1b72f559

Branch pushed to git repo; I updated commit sha1. New commits:

e9cb1cdCompParent: WIP coercion

comment:86 Changed 4 months ago by git

  • Commit changed from e9cb1cd84b5b65d97e86fc1680d2011c1b72f559 to d9912b9a68438f76eca9a2b02c3d68e218fcbdee

Branch pushed to git repo; I updated commit sha1. New commits:

d9912b9CompParent.tensor: Rename from tensor_product_sym, generalize to more than 2 factors

comment:87 Changed 4 months ago by git

  • Commit changed from d9912b9a68438f76eca9a2b02c3d68e218fcbdee to 5d189b274f6891057817fd2b227d6a9fb44b356d

Branch pushed to git repo; I updated commit sha1. New commits:

5d189b2Components: Coercion and pushouts

comment:88 Changed 4 months ago by git

  • Commit changed from 5d189b274f6891057817fd2b227d6a9fb44b356d to 3070443e0721262205766990dc924def33331636

Branch pushed to git repo; I updated commit sha1. New commits:

3070443CompParent: Fix up coercion

comment:89 Changed 4 months ago by mkoeppe

  • Authors set to Michael Jung, Matthias Koeppe
  • Dependencies changed from #31904 to #31904, #29619

comment:90 Changed 4 months ago by git

  • Commit changed from 3070443e0721262205766990dc924def33331636 to edd1572b835d188f66784fd0416fcd68e75a241c

Branch pushed to git repo; I updated commit sha1. New commits:

0ca670bsage.tensor.modules: Update doctest output for trivial output changes
02d6917VectorFieldModule.tensor_from_comp: Use CompParent*
c68734csage.manifolds: Update doctest output for trivial output changes
8ed01a7CompParent, Components_dict: Fix up module action
edd1572TensorField, ComponentsWithSym_dict: Remove comparison of _sym, _antisym with empty list, normalize to tuples

comment:91 Changed 4 months ago by mkoeppe

sage.tensor.modules passes all tests except for _test_not_implemented_methods (to be fixed by #29619)

There are still various errors in sage.manifolds. For example, in src/sage/manifolds/differentiable/multivectorfield.py:

sage -t --random-seed=0 src/sage/manifolds/differentiable/multivectorfield.py
**********************************************************************
File "src/sage/manifolds/differentiable/multivectorfield.py", line 117, in sage.manifolds.differentiable.multivectorfield.MultivectorField
Failed example:
    a.display(eU)
Expected:
    a = (x*y^2 + 2*x) d/dx/\d/dy
Got:
    a = (x*y^2 + 2*x) d/dx*d/dy + (-x*y^2 - 2*x) d/dy*d/dx

Help in spotting where these errors are coming from would be welcome

comment:92 Changed 4 months ago by gh-mjungmath

For some reason, the restrict method doesn't work properly:

sage: M = Manifold(2, 'M')
sage: U = M.open_subset('U') ; V = M.open_subset('V')
sage: M.declare_union(U,V)   # M is the union of U and V
sage: c_xy.<x,y> = U.chart() ; c_uv.<u,v> = V.chart()
sage: xy_to_uv = c_xy.transition_map(c_uv, (x+y, x-y),
....:                         intersection_name='W',
....:                         restrictions1= x>0, restrictions2= u+v>0)
sage: uv_to_xy = xy_to_uv.inverse()
sage: W = U.intersection(V)
sage: eU = c_xy.frame() ; eV = c_uv.frame()
sage: a = M.multivector_field(2, name='a')
sage: a[eU,0,1] = x*y^2 + 2*x
sage: a.add_comp_by_continuation(eV, W, c_uv)
sage: a.retrict(U)
Tensor field a of type (2,0) on the Open subset U of the 2-dimensional differentiable manifold M

But it should return

2-vector field a on the Open subset U of the 2-dimensional differentiable manifold M

comment:93 Changed 4 months ago by gh-mjungmath

Okay, I spotted the error. It's caused by these lines:

+        self._sym = tuple(self._sym)
+        self._antisym = tuple(self._antisym)

When a tensor field is displayed, it must be restricted to a parallelizable subset first. If that restriction does not exists, it's constructed from scratch. This is done by the tensor method of the corresponding vector field module of this subset. In particular, the parameters sym and antisym are passed to this method. In our particular case the code will be executed until

        elif tensor_type[0] > 1 and tensor_type[1] == 0 and antisym:
            if isinstance(antisym[0], (int, Integer)):
                # a single antisymmetry is provided as a tuple or a
                # range object; it is converted to a 1-item list:
                antisym = [tuple(antisym)]
            if isinstance(antisym, list):
                antisym0 = antisym[0]
            else:
                antisym0 = antisym
            if len(antisym0) == tensor_type[0]:
                return self.alternating_contravariant_tensor(
                                              tensor_type[0], name=name,
                                              latex_name=latex_name)

With the above change, however, len(antisym0) returns 1, but it should be 2 because the code turns [(0, 1)] into ((0,1),). So, this case will be ignored and a tensor without symmetries will be created instead.

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:94 Changed 4 months ago by gh-mjungmath

One way to solve this could be to adapt the preprocessing of antisym0 and sym0 in the tensor method, assuming antisym and sym is a tuple.

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:95 Changed 4 months ago by mkoeppe

Thanks for spotting this!

comment:96 Changed 4 months ago by git

  • Commit changed from edd1572b835d188f66784fd0416fcd68e75a241c to e5a81f253cea4e72c930e6d4a0fb0cc217b14f0e

Branch pushed to git repo; I updated commit sha1. New commits:

fef81a5FiniteRankFreeModule, VectorFieldModule: Accept both lists and tuples for sym, antisym
922b360TensorField, PseudoRiemannianMetric: Adjust to normalization of _sym, _antisym to tuples
e5a81f2sage.manifolds: Update doctest output for trivial output changes

comment:97 Changed 4 months ago by mkoeppe

With these changes, only failures that look like this remain:

sage -t --random-seed=0 src/sage/manifolds/differentiable/examples/sphere.py
**********************************************************************
File "src/sage/manifolds/differentiable/examples/sphere.py", line 56, in sage.manifolds.differentiable.examples.sphere
Failed example:
    h.display()
Exception raised:
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.manifolds.differentiable.examples.sphere[5]>", line 1, in <module>
        h.display()
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/manifolds/differentiable/tensorfield.py", line 1852, in display
        return rst.display(frame, chart)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/tensor/modules/free_module_tensor.py", line 723, in display
        coef = comp[ind_arg]
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/tensor/modules/comp_element_dict.py", line 2533, in __getitem__
        return self._output_formatter(self._comp[ind])
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/manifolds/scalarfield.py", line 1786, in coord_function
        raise ValueError("no starting chart could be found to " +
    ValueError: no starting chart could be found to compute the expression in the Chart (E^3, (x, y, z))

comment:98 Changed 4 months ago by mkoeppe

This is likely not the final form of this ticket.

The goals of this ticket -- one-time precomputation of data related to the symmetries -- have not been fully achieved yet because the parent class is now a module over a base ring. This is fine for numerical tensors that are all over the same ring (QQ or RDF) but for the symbolic tensors that are used in sage.manifolds, there are many base rings, so the symmetries will be recomputed for each of them.

But for now it was more important to me to have separate parents for separate base rings, as this will allow us to dispatch to different implementation backends (= element classes) - as part of @gh-honglizhaobob's project (#31991).

My solution for allowing more shared precomputation would be to introduce another class, similar to https://docs.sympy.org/latest/modules/tensor/tensor.html#sympy.tensor.tensor.TensorSymmetry, which only captures the symmetry group (with action).

Apart from this, there is still some code that should be pushed from element to parent, for example the symmetries from tensor contraction.

And there are branches in the code that can no longer be reached because the coercion system, via CompParent._element_constructor_, now guarantees that the inputs of single-underscore methods such as _add_ have the same symmetry already.

Also, I have not done any time measurements.

comment:99 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:100 Changed 4 months ago by mkoeppe

Further refactoring will go through #32029 (Action of a sympy TensorSymmetry)

comment:101 follow-up: Changed 4 months ago by gh-mjungmath

Looks good so far!

What about the idea with the morphisms in comment:28?

comment:102 in reply to: ↑ 101 Changed 4 months ago by mkoeppe

Replying to gh-mjungmath:

What about the idea with the morphisms in comment:28?

The next step into this direction should be to fix/complete the existing code for coercions.

sage: cp = CompParentWithSym(QQ, 4, sym=((1, 2), (3, 4)))
sage: cp2 = CompParentWithSym(QQ, 4, sym=((1, 2, 3, 4)))
sage: cp.coerce_map_from(cp2)   # returns None, should return injection to cp

Help with this is welcome!

comment:103 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5
Note: See TracTickets for help on using tickets.