Wrong parent when using diff_form of degree zero

[Michael Jung]

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.

[Michael Jung]

New commits:

​3b36f0d

Trac #29570: alternating_form returns correct element

[Travis Scrimshaw]

I am assuming this change is a mistake:

src/sage/tensor/modules/finite_rank_free_module.py

@@ -1084 +1084 @@
         See
         :class:`~sage.tensor.modules.ext_pow_free_module.ExtPowerDualFreeModule`
         for more documentation.
-
+def dual_exterior_power
         """
         from sage.tensor.modules.ext_pow_free_module import ExtPowerDualFreeModule
         if p == 0:

[git]

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

​85e7970

Trac #29570: Typo fixed, doctest added, returned element preferably non-zero

[Michael Jung]

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.

[Eric Gourgoulhon]

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.

[Michael Jung]

Replying to egourgoulhon:

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.

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).

[Eric Gourgoulhon]

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...

[Eric Gourgoulhon]

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...

[git]

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

​730046c	Merge branch 'develop' into t/29570/diff_form_bug
​130ae3f	Trac #29570: NotImplementedError for non-generic ring elements

[Michael Jung]

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.

[git]

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

​bba21e1

Trac #29570: Strange typo fixed...

[Eric Gourgoulhon]

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?

[git]

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

​ba3b4b9

Trac #29570: correct parent, vectorfield_module changes reverted

[Michael Jung]

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 in VectorFieldFreeModule?

[Eric Gourgoulhon]

Very good, thanks!