Opened 7 months ago

Closed 7 months 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) 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 7 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/diff_form_bug

comment:2 Changed 7 months ago by gh-mjungmath

  • Commit set to 3b36f0ddd3099d9da6b9eaaec12f2acc8877dcd7
  • Status changed from new to needs_review

New commits:

3b36f0dTrac #29570: alternating_form returns correct element

comment:3 Changed 7 months ago by tscrim

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): 
    10841084        See
    10851085        :class:`~sage.tensor.modules.ext_pow_free_module.ExtPowerDualFreeModule`
    10861086        for more documentation.
    1087 
     1087def dual_exterior_power
    10881088        """
    10891089        from sage.tensor.modules.ext_pow_free_module import ExtPowerDualFreeModule
    10901090        if p == 0:

comment:4 Changed 7 months ago by git

  • Commit changed from 3b36f0ddd3099d9da6b9eaaec12f2acc8877dcd7 to 85e79703f6cba59702b5aea32a8ce5516075eb8c

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

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

comment:5 Changed 7 months ago by gh-mjungmath

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: Changed 7 months ago by 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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 months ago by gh-mjungmath

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 months ago by 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...

comment:9 in reply to: ↑ 8 Changed 7 months ago by egourgoulhon

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 7 months ago by git

  • Commit changed from 85e79703f6cba59702b5aea32a8ce5516075eb8c to 130ae3fe8eb05abec4bf703e952f208fbbb87963

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

730046cMerge branch 'develop' into t/29570/diff_form_bug
130ae3fTrac #29570: NotImplementedError for non-generic ring elements

comment:11 follow-up: Changed 7 months ago by 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.

Last edited 7 months ago by gh-mjungmath (previous) (diff)

comment:12 Changed 7 months ago by git

  • Commit changed from 130ae3fe8eb05abec4bf703e952f208fbbb87963 to bba21e1439ca3fd2c6ef739358ab7b5dd95ebc12

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

bba21e1Trac #29570: Strange typo fixed...

comment:13 in reply to: ↑ 11 ; follow-up: Changed 7 months ago by egourgoulhon

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 7 months ago by git

  • Commit changed from bba21e1439ca3fd2c6ef739358ab7b5dd95ebc12 to ba3b4b94a777105073fc17f478bd480154c19ada

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

ba3b4b9Trac #29570: correct parent, vectorfield_module changes reverted

comment:15 in reply to: ↑ 13 Changed 7 months ago by gh-mjungmath

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?

comment:16 Changed 7 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
  • Status changed from needs_review to positive_review

Very good, thanks!

comment:17 Changed 7 months ago by vbraun

  • Branch changed from u/gh-mjungmath/diff_form_bug to ba3b4b94a777105073fc17f478bd480154c19ada
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.