Opened 22 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.7 |
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: |
Description (last modified by )
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 (105)
comment:1 Changed 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:2 Changed 16 months ago by
comment:3 Changed 16 months ago by
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 16 months ago by
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.
comment:5 Changed 16 months ago by
- Branch set to public/30307
comment:6 Changed 16 months ago by
- Commit set to 943f9ea4eb95d1e5b162ef88194799f1711d95c1
comment:7 follow-up: ↓ 9 Changed 16 months ago by
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 16 months ago by
- Commit changed from 943f9ea4eb95d1e5b162ef88194799f1711d95c1 to a2514b6e70a05dbb354b535d4acbfdde4f55fdb7
Branch pushed to git repo; I updated commit sha1. New commits:
a2514b6 | Trac #30307: no symmetries -> CompParent without symmetries
|
comment:9 in reply to: ↑ 7 ; follow-up: ↓ 11 Changed 16 months ago by
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: ↓ 13 Changed 16 months ago by
What would be a proper category for the parent?
comment:11 in reply to: ↑ 9 Changed 16 months ago by
Replying to mkoeppe:
Some more of the normalization happening in
CompParentWithSym.__init__
should probably be moved to__classcall_private__
For example?
comment:12 Changed 16 months ago by
For example the order of the component indices does not matter
comment:13 in reply to: ↑ 10 Changed 16 months ago by
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: ↓ 15 Changed 16 months ago by
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 16 months ago by
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 possibleComponents
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 16 months ago by
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: ↓ 18 ↓ 22 Changed 16 months ago by
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: ↓ 19 Changed 16 months ago by
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 16 months ago by
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 16 months ago by
Yes, that's clear. I meant, how to implement? Make CompParent
and Components
both simply extension types?
Would cdpef
ing 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 16 months ago by
- Commit changed from a2514b6e70a05dbb354b535d4acbfdde4f55fdb7 to fc22d5dd93320730cf97acfb426dedc6cba037c2
Branch pushed to git repo; I updated commit sha1. New commits:
fc22d5d | Trac 30307: Cythonize, restructuring, factory method to resemble old behavior
|
comment:22 in reply to: ↑ 17 Changed 16 months ago by
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:
fc22d5d | Trac 30307: Cythonize, restructuring, factory method to resemble old behavior
|
comment:23 Changed 16 months ago by
- Commit changed from fc22d5dd93320730cf97acfb426dedc6cba037c2 to 27bc9b1ddd376da516228b8c8144c6139376f061
Branch pushed to git repo; I updated commit sha1. New commits:
27bc9b1 | Trac #30307: pdx file + __init__.py removed
|
comment:24 follow-up: ↓ 30 Changed 16 months ago by
comment:25 Changed 16 months ago by
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 16 months ago by
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: ↓ 28 Changed 16 months ago by
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 16 months ago by
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".
comment:29 Changed 16 months ago by
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 16 months ago by
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 16 months ago by
Definitely! So, do we have a roadmap?
comment:32 Changed 16 months ago by
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 16 months ago by
@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: ↓ 35 Changed 16 months ago by
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 16 months ago by
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.
Can I take it as a "yes, what a wonderful idea!"?
comment:36 Changed 16 months ago by
Yes, there are many maps:
- For a
CompParent
with a coarser symmetry, there is an injection (lift
map) to anyCompParent
with a finer symmetry.
etc. (And yes, it's a wonderful idea.)
comment:37 Changed 16 months ago by
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 16 months ago by
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
.
comment:39 Changed 16 months ago by
To use morphisms, we need a new category (say Category of collections of abstract components
) and a homset, right?
comment:40 Changed 16 months ago by
Btw, I am not convinced that cpdef
ing 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.
comment:41 Changed 16 months ago by
comment:42 follow-up: ↓ 44 Changed 16 months ago by
What are you talking about cpdef
ing 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: ↓ 45 Changed 16 months ago by
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: ↓ 50 Changed 16 months ago by
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 16 months ago by
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 16 months ago by
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 16 months ago by
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: ↓ 49 Changed 16 months ago by
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: ↓ 51 Changed 16 months ago by
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 16 months ago by
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 16 months ago by
comment:52 follow-up: ↓ 53 Changed 16 months ago by
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 16 months ago by
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 16 months ago by
Mh. Matthias, Eric, what are your opinions on that __slot__
matter?
comment:55 follow-up: ↓ 56 Changed 16 months ago by
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 16 months ago by
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 15 months ago by
- 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: ↓ 59 Changed 15 months ago by
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 15 months ago by
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 14 months ago by
comment:61 Changed 14 months ago by
I'll return to it after the 9.3 release
comment:62 Changed 12 months ago by
- Description modified (diff)
comment:63 Changed 11 months ago by
- Cc gh-honglizhaobob added
comment:64 Changed 11 months ago by
- Commit changed from 27bc9b1ddd376da516228b8c8144c6139376f061 to 19f0a222e96da2206ec666a3008f7df51be96215
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
64a17ee | Trac #30307: CompParent and CompParentWithSym
|
2186b66 | Trac #30307: no symmetries -> CompParent without symmetries
|
08f3b7b | Trac 30307: Cythonize, restructuring, factory method to resemble old behavior
|
7c2a7c5 | Trac #30307: pdx file + __init__.py removed
|
19f0a22 | sage.tensor.modules.comp_element: Back to pure Python
|
comment:65 Changed 11 months ago by
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 11 months ago by
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 11 months ago by
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 11 months ago by
- Commit changed from 19f0a222e96da2206ec666a3008f7df51be96215 to 75aed16b63e8fee4e2ffbda9cd2a5d6b43fd52d5
Branch pushed to git repo; I updated commit sha1. New commits:
75aed16 | WIP: Remove dim and start_index from CompParent
|
comment:69 Changed 11 months ago by
- Commit changed from 75aed16b63e8fee4e2ffbda9cd2a5d6b43fd52d5 to 14a070f810a61a162908798f8869c7d1310ca76c
Branch pushed to git repo; I updated commit sha1. New commits:
14a070f | WIP: CompParent
|
comment:70 Changed 11 months ago by
- Commit changed from 14a070f810a61a162908798f8869c7d1310ca76c to f6bde0a9f9cfd69254aa0ca1ba8b0ffc04b77677
Branch pushed to git repo; I updated commit sha1. New commits:
f6bde0a | Finish comp_parent.py
|
comment:71 Changed 11 months ago by
- Description modified (diff)
comment:72 Changed 11 months ago by
- Commit changed from f6bde0a9f9cfd69254aa0ca1ba8b0ffc04b77677 to cabcd49408c1d08be982b5c36d2b1deec779e4f5
Branch pushed to git repo; I updated commit sha1. New commits:
cabcd49 | src/sage/tensor/modules/comp*.py: Mostly working
|
comment:73 Changed 11 months ago by
- Commit changed from cabcd49408c1d08be982b5c36d2b1deec779e4f5 to d8a7208977f3433f9a5f107f30326755cca71e9c
Branch pushed to git repo; I updated commit sha1. New commits:
d8a7208 | sage.tensor.modules.comp_element_dict: Split out from sage.tensor.modules.comp_element
|
comment:74 Changed 11 months ago by
Started to prepare the classes for multiple backend implementations.
comment:75 Changed 11 months ago by
- Commit changed from d8a7208977f3433f9a5f107f30326755cca71e9c to 5395aa37acf0d26543fa7c4d0f3950f23403684f
Branch pushed to git repo; I updated commit sha1. New commits:
5395aa3 | Update doctest output for trivial output changes
|
comment:76 Changed 11 months ago by
- Commit changed from 5395aa37acf0d26543fa7c4d0f3950f23403684f to 7d6ffede2fd7f191bcc03d506fa9b796304f592f
comment:77 Changed 11 months ago by
- Description modified (diff)
comment:78 Changed 11 months ago by
- Dependencies set to #31904
comment:79 Changed 11 months ago by
- Commit changed from 7d6ffede2fd7f191bcc03d506fa9b796304f592f to eed8f46a0eb7b7d25b9a754b2aa7b6f7853a8f7e
comment:80 Changed 11 months ago by
- Commit changed from eed8f46a0eb7b7d25b9a754b2aa7b6f7853a8f7e to 6da636ee3a3f0f615a01c22b1db61fd208dc819f
Branch pushed to git repo; I updated commit sha1. New commits:
6da636e | CompParent...: Add __classcall_private__ methods
|
comment:81 Changed 11 months ago by
- Commit changed from 6da636ee3a3f0f615a01c22b1db61fd208dc819f to 9c0d7f1231d02e9e9444b413f29a67e4deffd965
comment:82 Changed 11 months ago by
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 11 months ago by
- Commit changed from 9c0d7f1231d02e9e9444b413f29a67e4deffd965 to 4408f1de7b630547afec1c052c4e236570c99de9
comment:84 Changed 11 months ago by
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 11 months ago by
- Commit changed from 4408f1de7b630547afec1c052c4e236570c99de9 to e9cb1cd84b5b65d97e86fc1680d2011c1b72f559
Branch pushed to git repo; I updated commit sha1. New commits:
e9cb1cd | CompParent: WIP coercion
|
comment:86 Changed 11 months ago by
- Commit changed from e9cb1cd84b5b65d97e86fc1680d2011c1b72f559 to d9912b9a68438f76eca9a2b02c3d68e218fcbdee
Branch pushed to git repo; I updated commit sha1. New commits:
d9912b9 | CompParent.tensor: Rename from tensor_product_sym, generalize to more than 2 factors
|
comment:87 Changed 11 months ago by
- Commit changed from d9912b9a68438f76eca9a2b02c3d68e218fcbdee to 5d189b274f6891057817fd2b227d6a9fb44b356d
Branch pushed to git repo; I updated commit sha1. New commits:
5d189b2 | Components: Coercion and pushouts
|
comment:88 Changed 11 months ago by
- Commit changed from 5d189b274f6891057817fd2b227d6a9fb44b356d to 3070443e0721262205766990dc924def33331636
Branch pushed to git repo; I updated commit sha1. New commits:
3070443 | CompParent: Fix up coercion
|
comment:89 Changed 11 months ago by
- Dependencies changed from #31904 to #31904, #29619
comment:90 Changed 11 months ago by
- Commit changed from 3070443e0721262205766990dc924def33331636 to edd1572b835d188f66784fd0416fcd68e75a241c
Branch pushed to git repo; I updated commit sha1. New commits:
0ca670b | sage.tensor.modules: Update doctest output for trivial output changes
|
02d6917 | VectorFieldModule.tensor_from_comp: Use CompParent*
|
c68734c | sage.manifolds: Update doctest output for trivial output changes
|
8ed01a7 | CompParent, Components_dict: Fix up module action
|
edd1572 | TensorField, ComponentsWithSym_dict: Remove comparison of _sym, _antisym with empty list, normalize to tuples
|
comment:91 Changed 11 months ago by
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 11 months ago by
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 11 months ago by
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.
comment:94 Changed 11 months ago by
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.
comment:95 Changed 11 months ago by
Thanks for spotting this!
comment:96 Changed 11 months ago by
- Commit changed from edd1572b835d188f66784fd0416fcd68e75a241c to e5a81f253cea4e72c930e6d4a0fb0cc217b14f0e
Branch pushed to git repo; I updated commit sha1. New commits:
fef81a5 | FiniteRankFreeModule, VectorFieldModule: Accept both lists and tuples for sym, antisym
|
922b360 | TensorField, PseudoRiemannianMetric: Adjust to normalization of _sym, _antisym to tuples
|
e5a81f2 | sage.manifolds: Update doctest output for trivial output changes
|
comment:97 Changed 11 months ago by
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 11 months ago by
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 11 months ago by
- Description modified (diff)
comment:100 Changed 11 months ago by
Further refactoring will go through #32029 (Action of a sympy TensorSymmetry
)
comment:101 follow-up: ↓ 102 Changed 11 months ago by
Looks good so far!
What about the idea with the morphisms in comment:28?
comment:102 in reply to: ↑ 101 Changed 11 months ago by
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 10 months ago by
- Milestone changed from sage-9.4 to sage-9.5
comment:104 Changed 5 months ago by
- Milestone changed from sage-9.5 to sage-9.6
comment:105 Changed 3 months ago by
- Milestone changed from sage-9.6 to sage-9.7
Help with implementing this would be very welcome!