Opened 2 years ago
Closed 2 years ago
#29570 closed defect (fixed)
Wrong parent when using diff_form of degree zero
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.1 |
Component: | geometry | Keywords: | differential_forms |
Cc: | egourgoulhon, tscrim | 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 2-dimensional 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 2-dimensional 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 2 years ago by
- Branch set to u/gh-mjungmath/diff_form_bug
comment:2 Changed 2 years ago by
- Commit set to 3b36f0ddd3099d9da6b9eaaec12f2acc8877dcd7
- Status changed from new to needs_review
comment:3 Changed 2 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 2 years ago by
- Commit changed from 3b36f0ddd3099d9da6b9eaaec12f2acc8877dcd7 to 85e79703f6cba59702b5aea32a8ce5516075eb8c
Branch pushed to git repo; I updated commit sha1. New commits:
85e7970 | Trac #29570: Typo fixed, doctest added, returned element preferably non-zero
|
comment:5 Changed 2 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 follow-up: ↓ 7 Changed 2 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 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 2 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 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 2 years ago by
Replying to gh-mjungmath:
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 non-free 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 in reply to: ↑ 8 Changed 2 years ago by
Replying to egourgoulhon:
Replying to gh-mjungmath:
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 non-free 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 2 years ago by
- Commit changed from 85e79703f6cba59702b5aea32a8ce5516075eb8c to 130ae3fe8eb05abec4bf703e952f208fbbb87963
comment:11 follow-up: ↓ 13 Changed 2 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 2 years ago by
- Commit changed from 130ae3fe8eb05abec4bf703e952f208fbbb87963 to bba21e1439ca3fd2c6ef739358ab7b5dd95ebc12
Branch pushed to git repo; I updated commit sha1. New commits:
bba21e1 | Trac #29570: Strange typo fixed...
|
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 15 Changed 2 years ago by
Replying to gh-mjungmath:
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 2 years ago by
- Commit changed from bba21e1439ca3fd2c6ef739358ab7b5dd95ebc12 to ba3b4b94a777105073fc17f478bd480154c19ada
Branch pushed to git repo; I updated commit sha1. New commits:
ba3b4b9 | Trac #29570: correct parent, vectorfield_module changes reverted
|
comment:15 in reply to: ↑ 13 Changed 2 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 2 years ago by
- Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
- Status changed from needs_review to positive_review
Very good, thanks!
comment:17 Changed 2 years ago by
- Branch changed from u/gh-mjungmath/diff_form_bug to ba3b4b94a777105073fc17f478bd480154c19ada
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Trac #29570: alternating_form returns correct element