Opened 3 years ago
Closed 2 years ago
#26168 closed enhancement (fixed)
Add parallelism for two tensor calculus functions
Reported by:  mmancini  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  performance  Keywords:  tensor 
Cc:  egourgoulhon, slelievre  Merged in:  
Authors:  Marco Mancini  Reviewers:  Frédéric Chapoton, Eric Gourgoulhon, Samuel Lelièvre 
Report Upstream:  N/A  Work issues:  
Branch:  2ba9267 (Commits, GitHub, GitLab)  Commit:  2ba926778bf1fdc57229d260d07b2c449c478b0a 
Dependencies:  Stopgaps: 
Description (last modified by )
 Computation of tensor components in a new frame
In
FreeModuleTensor.components()
loopfor ind_new in new_comp.non_redundant_index_generator():
line 1101, filesrc/sage/tensor/modules/free_module_tensor.py
 Pullback of a tensor field
In
DiffMap.pullback()
loopfor ind_new in ptcomp.non_redundant_index_generator():
line 980, filesrc/sage/manifolds/differentiable/diff_map.py
Change History (28)
comment:1 Changed 3 years ago by
 Branch set to public/26168adding_parallelism_2_functions
 Commit set to 8b4f9a08855bbc92f09fcc7de15c86459d84ede6
comment:2 Changed 3 years ago by
 Description modified (diff)
comment:3 Changed 3 years ago by
 Description modified (diff)
comment:4 Changed 3 years ago by
 Description modified (diff)
comment:5 Changed 3 years ago by
 Commit changed from 8b4f9a08855bbc92f09fcc7de15c86459d84ede6 to 8a808dbc5db2a8859f763c4dc6ba679ceddc7fe3
comment:6 Changed 3 years ago by
 Commit changed from 8a808dbc5db2a8859f763c4dc6ba679ceddc7fe3 to 84c61a7cd2c00e8fe052ec88a8489fd3b59d5d3f
Branch pushed to git repo; I updated commit sha1. New commits:
84c61a7  _pullback_chart function in class DiffMap parallelized

comment:7 Changed 3 years ago by
 Cc slelievre added
 Description modified (diff)
 Summary changed from adding parallelism for two functions (tensor calculus) to Add parallelism for two tensor calculus functions
comment:8 Changed 2 years ago by
 Commit changed from 84c61a7cd2c00e8fe052ec88a8489fd3b59d5d3f to 5f8eaa62c979e058ea79f953ecafd8d44a61a8de
comment:9 Changed 2 years ago by
 Status changed from new to needs_review
Please, the loop functions in the title are parallelized. Please review this ticket.
comment:10 Changed 2 years ago by
For one instance of Parallel computation::
, a blank line is missing after it.
comment:11 Changed 2 years ago by
Doc does not build
missing empty line here:
+ Parallel computation:: + sage: Parallelism().set('tensor', nproc=2)
comment:12 followup: ↓ 13 Changed 2 years ago by
 Commit changed from 5f8eaa62c979e058ea79f953ecafd8d44a61a8de to a30055a8aba9b30826a61031da1c50267b4f39bd
Branch pushed to git repo; I updated commit sha1. New commits:
a30055a  Doc corrected

comment:13 in reply to: ↑ 12 ; followup: ↓ 15 Changed 2 years ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
a30055a Doc corrected
Could you please also correct the pyflakes error reported in https://patchbot.sagemath.org/log/26168/Gentoo%20Base%20System/2.2/x86_64/4.4.26gentoo/sage4/20181120%2013:42:02?plugin=pyflakes, namely
src/sage/manifolds/differentiable/diff_map.py:37: 'operator.itemgetter' imported but unused
comment:14 Changed 2 years ago by
 Commit changed from a30055a8aba9b30826a61031da1c50267b4f39bd to 2ec62c83490ba5268ef4bdd526121f8223f2cefa
Branch pushed to git repo; I updated commit sha1. New commits:
2ec62c8  deleted import of unused module

comment:15 in reply to: ↑ 13 Changed 2 years ago by
Replying to egourgoulhon:
import was deleted , thanks
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
a30055a Doc corrected
Could you please also correct the pyflakes error reported in https://patchbot.sagemath.org/log/26168/Gentoo%20Base%20System/2.2/x86_64/4.4.26gentoo/sage4/20181120%2013:42:02?plugin=pyflakes, namely
src/sage/manifolds/differentiable/diff_map.py:37: 'operator.itemgetter' imported but unused
comment:16 followup: ↓ 20 Changed 2 years ago by
The doctests regarding the change of basis in src/sage/tensor/modules/free_module_tensor.py
are not valid because the tensor components are cached. One must introduce a copy of the tensor t
to trigger a new computation of the components in the basis f
, i.e. one shall write something like
sage: Parallelism().set('tensor', nproc=2) sage: t2 = v*w # identical to t but initialized only in basis e sage: t2.display(f) v*w = 6 f_1*f^1  20/3 f_1*f^2 + 27/8 f_2*f^1 + 15/4 f_2*f^2 sage: t2[f,:] == t[f,:] # check of the parallel computation True sage: Parallelism().set('tensor', nproc=1) # switch off parallelization
But then t2.display(f)
results in
ValueError: Cannot pickle code objects from closures
The doctest in display_comp()
is also not valid, but I suggest to simply remove it because it is redundant with that in display()
.
comment:17 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:18 Changed 2 years ago by
 Commit changed from 2ec62c83490ba5268ef4bdd526121f8223f2cefa to d1fb9d50e15045de359a70a28ef373f1bdb25c3e
Branch pushed to git repo; I updated commit sha1. New commits:
d1fb9d5  Error in parallel part: corrected. DocTest improved

comment:19 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:20 in reply to: ↑ 16 ; followup: ↓ 21 Changed 2 years ago by
Replying to egourgoulhon: There was an error in the parallel function: it is corrected. I also applied the suggested changes in the doctest thank
The doctests regarding the change of basis in
src/sage/tensor/modules/free_module_tensor.py
are not valid because the tensor components are cached. One must introduce a copy of the tensort
to trigger a new computation of the components in the basisf
, i.e. one shall write something likesage: Parallelism().set('tensor', nproc=2) sage: t2 = v*w # identical to t but initialized only in basis e sage: t2.display(f) v*w = 6 f_1*f^1  20/3 f_1*f^2 + 27/8 f_2*f^1 + 15/4 f_2*f^2 sage: t2[f,:] == t[f,:] # check of the parallel computation True sage: Parallelism().set('tensor', nproc=1) # switch off parallelizationBut then
t2.display(f)
results inValueError: Cannot pickle code objects from closuresThe doctest in
display_comp()
is also not valid, but I suggest to simply remove it because it is redundant with that indisplay()
.
comment:21 in reply to: ↑ 20 ; followup: ↓ 23 Changed 2 years ago by
Replying to mmancini:
Replying to egourgoulhon: There was an error in the parallel function: it is corrected. I also applied the suggested changes in the doctest
Thanks.
The doctest in
display_comp()
is also not valid, but I suggest to simply remove it because it is redundant with that indisplay()
.
Could you remove that doctest?
comment:22 Changed 2 years ago by
 Commit changed from d1fb9d50e15045de359a70a28ef373f1bdb25c3e to 2ba926778bf1fdc57229d260d07b2c449c478b0a
Branch pushed to git repo; I updated commit sha1. New commits:
2ba9267  removed ridundant test

comment:23 in reply to: ↑ 21 ; followup: ↓ 24 Changed 2 years ago by
Replying to egourgoulhon:
Replying to mmancini:
Replying to egourgoulhon: There was an error in the parallel function: it is corrected. I also applied the suggested changes in the doctest
Thanks.
The doctest in
display_comp()
is also not valid, but I suggest to simply remove it because it is redundant with that indisplay()
.Could you remove that doctest?
Done.
comment:24 in reply to: ↑ 23 ; followup: ↓ 27 Changed 2 years ago by
Replying to mmancini:
Could you remove that doctest?
Done.
Thanks. I've tested the changes both in python2 and python3. Everything is OK and I thing it is ready to go. Thanks for this improvement!
comment:25 Changed 2 years ago by
 Reviewers set to Frédéric Chapoton, Eric Gourgoulhon, Samuel Lelièvre
 Status changed from needs_review to positive_review
comment:26 Changed 2 years ago by
 Milestone changed from sage8.4 to sage8.5
comment:27 in reply to: ↑ 24 Changed 2 years ago by
Replying to egourgoulhon:
Thanks to you for the review
Replying to mmancini:
Could you remove that doctest?
Done.
Thanks. I've tested the changes both in python2 and python3. Everything is OK and I thing it is ready to go. Thanks for this improvement!
comment:28 Changed 2 years ago by
 Branch changed from public/26168adding_parallelism_2_functions to 2ba926778bf1fdc57229d260d07b2c449c478b0a
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits: