Opened 3 years ago
Closed 3 years ago
#29570 closed defect (fixed)
Wrong parent when using diff_form of degree zero
Reported by:  Michael Jung  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  geometry  Keywords:  differential_forms 
Cc:  Eric Gourgoulhon, Travis Scrimshaw  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  ba3b4b9 (Commits, GitHub, GitLab)  Commit:  ba3b4b94a777105073fc17f478bd480154c19ada 
Dependencies:  Stopgaps: 
Description
sage: M = Manifold(2, 'M') sage: X.<x,y> = M.chart() sage: M.diff_form(0).parent() Free module X(M) of vector fields on the 2dimensional differentiable manifold M
This output is obviously wrong. The parent should represent the algebra of scalar fields instead. However, the element is correct:
sage: M.diff_form(0) Scalar field on the 2dimensional differentiable manifold M
While seeking the origin, I have encountered an issue even worse:
from sage.tensor.modules.finite_rank_free_module import FiniteRankFreeModule sage: M = FiniteRankFreeModule(QQ, 3) sage: M.alternating_form(0)  KeyError Traceback (most recent call last) ... AttributeError: 'RationalField_with_category' object has no attribute 'element_class'
This should be fixed with this ticket.
Change History (17)
comment:1 Changed 3 years ago by
Branch:  → u/ghmjungmath/diff_form_bug 

comment:2 Changed 3 years ago by
Commit:  → 3b36f0ddd3099d9da6b9eaaec12f2acc8877dcd7 

Status:  new → needs_review 
comment:3 Changed 3 years ago by
I am assuming this change is a mistake:

src/sage/tensor/modules/finite_rank_free_module.py
diff git a/src/sage/tensor/modules/finite_rank_free_module.py b/src/sage/tensor/modules/finite_rank_free_module.py index b8d048b..623454c 100644
a b class FiniteRankFreeModule(UniqueRepresentation, Parent): 1084 1084 See 1085 1085 :class:`~sage.tensor.modules.ext_pow_free_module.ExtPowerDualFreeModule` 1086 1086 for more documentation. 1087 1087 def dual_exterior_power 1088 1088 """ 1089 1089 from sage.tensor.modules.ext_pow_free_module import ExtPowerDualFreeModule 1090 1090 if p == 0:
comment:4 Changed 3 years ago by
Commit:  3b36f0ddd3099d9da6b9eaaec12f2acc8877dcd7 → 85e79703f6cba59702b5aea32a8ce5516075eb8c 

Branch pushed to git repo; I updated commit sha1. New commits:
85e7970  Trac #29570: Typo fixed, doctest added, returned element preferably nonzero

comment:5 Changed 3 years ago by
Sure. I am sorry. Yesterday was late for me...
I removed the unnecessary line and also added a doctest to show that the error had been fixed.
comment:6 followup: 7 Changed 3 years ago by
Thanks for fixing this.
However, I don't think self._ring._an_element_()
shall be used when degree
is 0 in FiniteRankFreeModule.alternating_form()
. Indeed, the method _an_element()
returns a random element (for instance 1
in your example), whereas alternating_form()
shall return a generic element, as it does for the degrees >= 1.
Maybe a try
with the element_class
and except
with the generic constructor of the base ring shall be better.
comment:7 followup: 8 Changed 3 years ago by
Replying to egourgoulhon:
Thanks for fixing this. However, I don't think
self._ring._an_element_()
shall be used whendegree
is 0 inFiniteRankFreeModule.alternating_form()
. Indeed, the method_an_element()
returns a random element (for instance1
in your example), whereasalternating_form()
shall return a generic element, as it does for the degrees >= 1. Maybe atry
with theelement_class
andexcept
with the generic constructor of the base ring shall be better.
I agree, your suggestions sounds much better. Do arbitrary rings such as ZZ
even provide constructors for generic elements?
However, in the first place, this issue occured due to the line
return self.dual_exterior_power(degree).element_class(self, degree, name=name, latex_name=latex_name)
where self
is definitely not the parent of the ring element. I would say, this is highly unstable since we cannot predict which arguments the element class actually takes and needs (for degree > 0 this is fine since we know which elements we deal with).
comment:8 followup: 9 Changed 3 years ago by
Replying to ghmjungmath:
I would say, this is highly unstable since we cannot predict which arguments the element class actually takes and needs (for degree > 0 this is fine since we know which elements we deal with).
Indeed! Maybe an explicit scalar field construction (inside a try block) like you did for the nonfree case is preferable then:
try: return self._domain.scalar_field(name=name, latex_name=latex_name) except AttributeError: return self._ring.an_element() # or something better...
comment:9 Changed 3 years ago by
Replying to egourgoulhon:
Replying to ghmjungmath:
I would say, this is highly unstable since we cannot predict which arguments the element class actually takes and needs (for degree > 0 this is fine since we know which elements we deal with).
Indeed! Maybe an explicit scalar field construction (inside a try block) like you did for the nonfree case is preferable then:
try: return self._domain.scalar_field(name=name, latex_name=latex_name) except AttributeError: return self._ring.an_element() # or something better...
Thinking about it, what about having the except AttributeError
block do raise NotImplementedError
? Sounds better than returning a random element...
comment:10 Changed 3 years ago by
Commit:  85e79703f6cba59702b5aea32a8ce5516075eb8c → 130ae3fe8eb05abec4bf703e952f208fbbb87963 

comment:11 followup: 13 Changed 3 years ago by
Something like this?
I feel uncomfortable to add the special case of scalar fields to the most general case of modules over arbitrary ring. For that reason, I added this particular case to the vector field module.
comment:12 Changed 3 years ago by
Commit:  130ae3fe8eb05abec4bf703e952f208fbbb87963 → bba21e1439ca3fd2c6ef739358ab7b5dd95ebc12 

Branch pushed to git repo; I updated commit sha1. New commits:
bba21e1  Trac #29570: Strange typo fixed...

comment:13 followup: 15 Changed 3 years ago by
Replying to ghmjungmath:
Something like this?
I feel uncomfortable to add the special case of scalar fields to the most general case of modules over arbitrary ring. For that reason, I added this particular case to the vector field module.
Sounds better indeed. Probably
return self._ring.element_class(self, name=name, latex_name=latex_name)
must be replaced by
return self._ring.element_class(self._ring, name=name, latex_name=latex_name)
Then, do you really need to redefine alternating_form
in VectorFieldFreeModule
?
comment:14 Changed 3 years ago by
Commit:  bba21e1439ca3fd2c6ef739358ab7b5dd95ebc12 → ba3b4b94a777105073fc17f478bd480154c19ada 

Branch pushed to git repo; I updated commit sha1. New commits:
ba3b4b9  Trac #29570: correct parent, vectorfield_module changes reverted

comment:15 Changed 3 years ago by
This is much better! Changed it.
Replying to egourgoulhon:
Probably
return self._ring.element_class(self, name=name, latex_name=latex_name)must be replaced by
return self._ring.element_class(self._ring, name=name, latex_name=latex_name)Then, do you really need to redefine
alternating_form
inVectorFieldFreeModule
?
comment:16 Changed 3 years ago by
Reviewers:  → Eric Gourgoulhon, Travis Scrimshaw 

Status:  needs_review → positive_review 
Very good, thanks!
comment:17 Changed 3 years ago by
Branch:  u/ghmjungmath/diff_form_bug → ba3b4b94a777105073fc17f478bd480154c19ada 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Trac #29570: alternating_form returns correct element